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.

Author: kripken

src/cfg/Relooper.cpp

@@ -804,7 +804,7 @@ struct Optimizer : public RelooperRecursor {
       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);

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,

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 =

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,31 +172,13 @@ 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);
     }
   }
 
   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;
     }

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;
     }

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);

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);

src/passes/StackIR.cpp

@@ -238,7 +238,7 @@ class StackIROptimizer {
         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;
         }

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) {

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