Remove 'none' type as a branch target in ReFinalize (#2492)

That was needed for super-old wasm type system, where we allowed

(block $x
 (br_if $x
   (unreachable)
   (nop)
 )
)

That is, we differentiated "taken" branches from "named" ones (just
referred to by name, but not actually taken as it's in unreachable code).

We don't need to differentiate those any more. Remove the ReFinalize
code that considered it, and also remove the named/taken distinction in
other places.

13 files changed, 21 insertions, 100 deletions

diff --git a/src/cfg/Relooper.cpp b/src/cfg/Relooper.cpp
index 08f28bc30..3acb2a2bf 100644
--- a/src/cfg/Relooper.cpp
+++ b/src/cfg/Relooper.cpp
@@ -804,7 +804,7 @@ private:
       Outer = Builder.makeBlock(Curr);
     } else if (Outer->name.is()) {
       // Perhaps the name can be removed.
-      if (!wasm::BranchUtils::BranchSeeker::hasNamed(Outer, Outer->name)) {
+      if (!wasm::BranchUtils::BranchSeeker::has(Outer, Outer->name)) {
         Outer->name = wasm::Name();
       } else {
         Outer = Builder.makeBlock(Curr);
diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp
index 007a7ee55..bc5522c10 100644
--- a/src/ir/ReFinalize.cpp
+++ b/src/ir/ReFinalize.cpp
@@ -44,41 +44,12 @@ void ReFinalize::visitBlock(Block* curr) {
     curr->type = none;
     return;
   }
-  auto old = curr->type;
   // do this quickly, without any validation
   // last element determines type
   curr->type = curr->list.back()->type;
   // if concrete, it doesn't matter if we have an unreachable child, and we
   // don't need to look at breaks
   if (curr->type.isConcrete()) {
-    // make sure our branches make sense for us - we may have just made
-    // ourselves concrete for a value flowing out, while branches did not send a
-    // value. such branches could not have been actually taken before, that is,
-    // there were in unreachable code, but we do still need to fix them up here.
-    if (!old.isConcrete()) {
-      auto iter = breakValues.find(curr->name);
-      if (iter != breakValues.end()) {
-        // there is a break to here
-        auto type = iter->second;
-        if (type == none) {
-          // we need to fix this up. set the values to unreachables
-          // note that we don't need to handle br_on_exn here, because its value
-          // type is never none
-          for (auto* br : FindAll<Break>(curr).list) {
-            handleBranchForVisitBlock(br, curr->name, getModule());
-          }
-          for (auto* sw : FindAll<Switch>(curr).list) {
-            handleBranchForVisitBlock(sw, curr->name, getModule());
-          }
-          // and we need to propagate that type out, re-walk
-          ReFinalize fixer;
-          fixer.setModule(getModule());
-          Expression* temp = curr;
-          fixer.walk(temp);
-          assert(temp == curr);
-        }
-      }
-    }
     return;
   }
   // otherwise, we have no final fallthrough element to determine the type,
diff --git a/src/ir/block-utils.h b/src/ir/block-utils.h
index 1ed9ee413..ca8b7179b 100644
--- a/src/ir/block-utils.h
+++ b/src/ir/block-utils.h
@@ -34,7 +34,7 @@ inline Expression*
 simplifyToContents(Block* block, T* parent, bool allowTypeChange = false) {
   auto& list = block->list;
   if (list.size() == 1 &&
-      !BranchUtils::BranchSeeker::hasNamed(list[0], block->name)) {
+      !BranchUtils::BranchSeeker::has(list[0], block->name)) {
     // just one element. try to replace the block
     auto* singleton = list[0];
     auto sideEffects =
diff --git a/src/ir/branch-utils.h b/src/ir/branch-utils.h
index c4ff26c0c..53279eee6 100644
--- a/src/ir/branch-utils.h
+++ b/src/ir/branch-utils.h
@@ -151,12 +151,8 @@ inline std::set<Name> getBranchTargets(Expression* ast) {
 // Finds if there are branches targeting a name. Note that since names are
 // unique in our IR, we just need to look for the name, and do not need
 // to analyze scoping.
-// By default we consider all branches, so any place there is a branch that
-// names the target. You can unset 'named' to only note branches that appear
-// reachable (i.e., are not obviously unreachable).
 struct BranchSeeker : public PostWalker<BranchSeeker> {
   Name target;
-  bool named = true;
 
   Index found = 0;
   Type valueType;
@@ -176,15 +172,6 @@ struct BranchSeeker : public PostWalker<BranchSeeker> {
   }
 
   void visitBreak(Break* curr) {
-    if (!named) {
-      // ignore an unreachable break
-      if (curr->condition && curr->condition->type == unreachable) {
-        return;
-      }
-      if (curr->value && curr->value->type == unreachable) {
-        return;
-      }
-    }
     // check the break
     if (curr->name == target) {
       noteFound(curr->value);
@@ -192,15 +179,6 @@ struct BranchSeeker : public PostWalker<BranchSeeker> {
   }
 
   void visitSwitch(Switch* curr) {
-    if (!named) {
-      // ignore an unreachable switch
-      if (curr->condition->type == unreachable) {
-        return;
-      }
-      if (curr->value && curr->value->type == unreachable) {
-        return;
-      }
-    }
     // check the switch
     for (auto name : curr->targets) {
       if (name == target) {
@@ -213,39 +191,13 @@ struct BranchSeeker : public PostWalker<BranchSeeker> {
   }
 
   void visitBrOnExn(BrOnExn* curr) {
-    if (!named) {
-      // ignore an unreachable br_on_exn
-      if (curr->exnref->type == unreachable) {
-        return;
-      }
-    }
     // check the br_on_exn
     if (curr->name == target) {
       noteFound(curr->sent);
     }
   }
 
-  static bool hasReachable(Expression* tree, Name target) {
-    if (!target.is()) {
-      return false;
-    }
-    BranchSeeker seeker(target);
-    seeker.named = false;
-    seeker.walk(tree);
-    return seeker.found > 0;
-  }
-
-  static Index countReachable(Expression* tree, Name target) {
-    if (!target.is()) {
-      return 0;
-    }
-    BranchSeeker seeker(target);
-    seeker.named = false;
-    seeker.walk(tree);
-    return seeker.found;
-  }
-
-  static bool hasNamed(Expression* tree, Name target) {
+  static bool has(Expression* tree, Name target) {
     if (!target.is()) {
       return false;
     }
@@ -254,7 +206,7 @@ struct BranchSeeker : public PostWalker<BranchSeeker> {
     return seeker.found > 0;
   }
 
-  static Index countNamed(Expression* tree, Name target) {
+  static Index count(Expression* tree, Name target) {
     if (!target.is()) {
       return 0;
     }
diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp
index 058d9e4e4..dcbac960e 100644
--- a/src/passes/DeadCodeElimination.cpp
+++ b/src/passes/DeadCodeElimination.cpp
@@ -195,7 +195,7 @@ struct DeadCodeElimination
       reachableBreaks.erase(curr->name);
     }
     if (isUnreachable(curr->body) &&
-        !BranchUtils::BranchSeeker::hasNamed(curr->body, curr->name)) {
+        !BranchUtils::BranchSeeker::has(curr->body, curr->name)) {
       replaceCurrent(curr->body);
       return;
     }
diff --git a/src/passes/MergeBlocks.cpp b/src/passes/MergeBlocks.cpp
index babbf77ba..8e04ed47f 100644
--- a/src/passes/MergeBlocks.cpp
+++ b/src/passes/MergeBlocks.cpp
@@ -287,7 +287,7 @@ optimizeBlock(Block* curr, Module* module, PassOptions& passOptions) {
         auto childName = childBlock->name;
         for (size_t j = 0; j < childSize; j++) {
           auto* item = childList[j];
-          if (BranchUtils::BranchSeeker::hasNamed(item, childName)) {
+          if (BranchUtils::BranchSeeker::has(item, childName)) {
             // We can't remove this from the child.
             keepStart = j;
             keepEnd = childSize;
@@ -300,7 +300,7 @@ optimizeBlock(Block* curr, Module* module, PassOptions& passOptions) {
         auto childName = loop->name;
         for (auto j = int(childSize - 1); j >= 0; j--) {
           auto* item = childList[j];
-          if (BranchUtils::BranchSeeker::hasNamed(item, childName)) {
+          if (BranchUtils::BranchSeeker::has(item, childName)) {
             // We can't remove this from the child.
             keepStart = 0;
             keepEnd = std::max(Index(j + 1), keepEnd);
diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index d8668196d..55f57302d 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -504,8 +504,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             //  (b) this br_if is the only branch to that block (so the block
             //      will vanish)
             if (brIf->name == block->name &&
-                BranchUtils::BranchSeeker::countNamed(block, block->name) ==
-                  1) {
+                BranchUtils::BranchSeeker::count(block, block->name) == 1) {
               // note that we could drop the last element here, it is a br we
               // know for sure is removable, but telling stealSlice to steal all
               // to the end is more efficient, it can just truncate.
@@ -566,16 +565,16 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             worked = true;
           } else if (auto* iff = curr->list[0]->dynCast<If>()) {
             // The label can't be used in the condition.
-            if (BranchUtils::BranchSeeker::countNamed(iff->condition,
-                                                      curr->name) == 0) {
+            if (BranchUtils::BranchSeeker::count(iff->condition, curr->name) ==
+                0) {
               // We can move the block into either arm, if there are no uses in
               // the other.
               Expression** target = nullptr;
-              if (!iff->ifFalse || BranchUtils::BranchSeeker::countNamed(
+              if (!iff->ifFalse || BranchUtils::BranchSeeker::count(
                                      iff->ifFalse, curr->name) == 0) {
                 target = &iff->ifTrue;
-              } else if (BranchUtils::BranchSeeker::countNamed(
-                           iff->ifTrue, curr->name) == 0) {
+              } else if (BranchUtils::BranchSeeker::count(iff->ifTrue,
+                                                          curr->name) == 0) {
                 target = &iff->ifFalse;
               }
               if (target) {
@@ -874,7 +873,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
           // can ignore.
           if (br && br->condition && br->name == curr->name &&
               br->type != unreachable) {
-            if (BranchUtils::BranchSeeker::countNamed(curr, curr->name) == 1) {
+            if (BranchUtils::BranchSeeker::count(curr, curr->name) == 1) {
               // no other breaks to that name, so we can do this
               if (!drop) {
                 assert(!br->value);
diff --git a/src/passes/StackIR.cpp b/src/passes/StackIR.cpp
index 2434c44f6..e4aa55d68 100644
--- a/src/passes/StackIR.cpp
+++ b/src/passes/StackIR.cpp
@@ -238,7 +238,7 @@ private:
         continue;
       }
       if (auto* block = inst->origin->dynCast<Block>()) {
-        if (!BranchUtils::BranchSeeker::hasNamed(block, block->name)) {
+        if (!BranchUtils::BranchSeeker::has(block, block->name)) {
           // TODO optimize, maybe run remove-unused-names
           inst = nullptr;
         }
diff --git a/src/passes/Vacuum.cpp b/src/passes/Vacuum.cpp
index d97ce461b..c2911f2b2 100644
--- a/src/passes/Vacuum.cpp
+++ b/src/passes/Vacuum.cpp
@@ -364,7 +364,6 @@ struct Vacuum : public WalkerPass<ExpressionStackWalker<Vacuum>> {
           bool canPop = true;
           if (block->name.is()) {
             BranchUtils::BranchSeeker seeker(block->name);
-            seeker.named = true;
             Expression* temp = block;
             seeker.walk(temp);
             if (seeker.found && seeker.valueType != none) {
diff --git a/src/tools/wasm-reduce.cpp b/src/tools/wasm-reduce.cpp
index f1fbebc24..0fe15b444 100644
--- a/src/tools/wasm-reduce.cpp
+++ b/src/tools/wasm-reduce.cpp
@@ -517,7 +517,7 @@ struct Reducer
       // replace a singleton
       auto& list = block->list;
       if (list.size() == 1 &&
-          !BranchUtils::BranchSeeker::hasNamed(block, block->name)) {
+          !BranchUtils::BranchSeeker::has(block, block->name)) {
         if (tryToReplaceCurrent(block->list[0])) {
           return;
         }
@@ -549,7 +549,7 @@ struct Reducer
       return; // nothing more to do
     } else if (auto* loop = curr->dynCast<Loop>()) {
       if (shouldTryToReduce() &&
-          !BranchUtils::BranchSeeker::hasNamed(loop, loop->name)) {
+          !BranchUtils::BranchSeeker::has(loop, loop->name)) {
         tryToReplaceCurrent(loop->body);
       }
       return; // nothing more to do
diff --git a/src/wasm-stack.h b/src/wasm-stack.h
index 26105ccdc..7564fbcdb 100644
--- a/src/wasm-stack.h
+++ b/src/wasm-stack.h
@@ -237,7 +237,7 @@ template<typename SubType> void BinaryenIRWriter<SubType>::write() {
 template<typename SubType>
 void BinaryenIRWriter<SubType>::visitPossibleBlockContents(Expression* curr) {
   auto* block = curr->dynCast<Block>();
-  if (!block || BranchUtils::BranchSeeker::hasNamed(block, block->name)) {
+  if (!block || BranchUtils::BranchSeeker::has(block, block->name)) {
     visit(curr);
     return;
   }
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index 44b08f4a3..24a4fcb35 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -1593,7 +1593,7 @@ Expression* SExpressionWasmBuilder::makeIf(Element& s) {
   ret->finalize(type);
   nameMapper.popLabelName(label);
   // create a break target if we must
-  if (BranchUtils::BranchSeeker::hasNamed(ret, label)) {
+  if (BranchUtils::BranchSeeker::has(ret, label)) {
     auto* block = allocator.alloc<Block>();
     block->name = label;
     block->list.push_back(ret);
@@ -1798,7 +1798,7 @@ Expression* SExpressionWasmBuilder::makeTry(Element& s) {
   ret->finalize(type);
   nameMapper.popLabelName(label);
   // create a break target if we must
-  if (BranchUtils::BranchSeeker::hasNamed(ret, label)) {
+  if (BranchUtils::BranchSeeker::has(ret, label)) {
     auto* block = allocator.alloc<Block>();
     block->name = label;
     block->list.push_back(ret);
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
index 6e5f1fdf5..f0c853172 100644
--- a/src/wasm/wasm.cpp
+++ b/src/wasm/wasm.cpp
@@ -292,7 +292,7 @@ static void handleUnreachable(Block* block,
       // there is an unreachable child, so we are unreachable, unless we have a
       // break
       if (!breakabilityKnown) {
-        hasBreak = BranchUtils::BranchSeeker::hasNamed(block, block->name);
+        hasBreak = BranchUtils::BranchSeeker::has(block, block->name);
       }
       if (!hasBreak) {
         block->type = unreachable;