Allow pops to validate inside unnamed blocks, removing almost all handleBlockNestedPop() calls

src/ir/ReFinalize.cpp

@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-#include "ir/eh-utils.h"
 #include "ir/utils.h"
 
 namespace wasm {
@@ -140,10 +139,7 @@ void ReFinalize::visitRethrow(Rethrow* curr) { curr->finalize(); }
 void ReFinalize::visitThrowRef(ThrowRef* curr) { curr->finalize(); }
 void ReFinalize::visitNop(Nop* curr) { curr->finalize(); }
 void ReFinalize::visitUnreachable(Unreachable* curr) { curr->finalize(); }
-void ReFinalize::visitPop(Pop* curr) {
-  curr->finalize();
-  seenPop = true;
-}
+void ReFinalize::visitPop(Pop* curr) { curr->finalize(); }
 void ReFinalize::visitTupleMake(TupleMake* curr) { curr->finalize(); }
 void ReFinalize::visitTupleExtract(TupleExtract* curr) { curr->finalize(); }
 void ReFinalize::visitRefI31(RefI31* curr) { curr->finalize(); }
@@ -224,11 +220,7 @@ void ReFinalize::visitDataSegment(DataSegment* curr) {
   WASM_UNREACHABLE("unimp");
 }
 void ReFinalize::visitTag(Tag* curr) { WASM_UNREACHABLE("unimp"); }
-void ReFinalize::visitFunction(Function* curr) {
-  if (seenPop && addedBlock) {
-    EHUtils::handleBlockNestedPops(curr, *getModule());
-  }
-}
+void ReFinalize::visitFunction(Function* curr) {}
 void ReFinalize::visitModule(Module* curr) { WASM_UNREACHABLE("unimp"); }
 
 void ReFinalize::updateBreakValueType(Name name, Type type) {
@@ -251,7 +243,6 @@ void ReFinalize::replaceUntaken(Expression* value, Expression* otherChild) {
       otherChild = builder.makeDrop(otherChild);
     }
     replaceCurrent(builder.makeBlock({value, otherChild}, Type::unreachable));
-    addedBlock = true;
   } else if (value) {
     replaceCurrent(value);
   } else {

src/ir/eh-utils.cpp

@@ -15,8 +15,10 @@
  */
 
 #include "ir/eh-utils.h"
-#include "ir/branch-utils.h"
 #include "ir/find_all.h"
+#include "ir/iteration.h"
+#include "ir/properties.h"
+#include "wasm-builder.h"
 
 namespace wasm {
 
@@ -37,9 +39,6 @@ getFirstPop(Expression* catchBody, bool& isPopNested, Expression**& popPtr) {
   Expression* firstChild = catchBody;
   isPopNested = false;
   popPtr = nullptr;
-  // When there are multiple expressions within a catch body, an implicit
-  // block is created within it for convenience purposes.
-  auto* implicitBlock = catchBody->dynCast<Block>();
 
   // Go down the line for the first child until we reach a leaf. A pop should be
   // in that first-descendant line.
@@ -64,24 +63,19 @@ getFirstPop(Expression* catchBody, bool& isPopNested, Expression**& popPtr) {
         // run more than once
         return nullptr;
       }
-      if (firstChild->is<Block>()) {
-        // If there are no branches that targets the implicit block, it will be
-        // removed when written back. But if there are branches that target the
-        // implicit block,
+      if (auto* block = firstChild->dynCast<Block>()) {
+        // An unnamed block will be removed when written back. But if the block
+        // has a name,
         // (catch $e
         //   (block $l0
         //     (pop i32) ;; within a block!
         //     (br $l0)
         //     ...
         //   )
-        // This cannot be removed, so this is considered a nested pop (which we
-        // should fix).
-        if (firstChild == implicitBlock) {
-          if (BranchUtils::BranchSeeker::has(implicitBlock,
-                                             implicitBlock->name)) {
-            isPopNested = true;
-          }
-        } else {
+        // )
+        // It may be emitted in the binary format, so this is considered a
+        // nested pop (which we should fix).
+        if (block->name.is()) {
           isPopNested = true;
         }
       } else if (firstChild->is<Try>() || firstChild->is<TryTable>()) {

src/ir/utils.h

@@ -154,10 +154,6 @@ struct ReFinalize
   // Replace an untaken branch/switch with an unreachable value.
   // A condition may also exist and may or may not be unreachable.
   void replaceUntaken(Expression* value, Expression* condition);
-
-  // If we see a pop and also add a block, we need to do EH fixups.
-  bool seenPop = false;
-  bool addedBlock = false;
 };
 
 // Re-finalize a single node. This is slow, if you want to refinalize

src/passes/AbstractTypeRefining.cpp

@@ -36,7 +36,6 @@
 #include <memory>
 
 #include "ir/drop.h"
-#include "ir/eh-utils.h"
 #include "ir/localize.h"
 #include "ir/module-utils.h"
 #include "ir/subtypes.h"
@@ -320,7 +319,6 @@ struct AbstractTypeRefining : public Pass {
 
     struct Preoptimizer : WalkerPass<PostWalker<Preoptimizer>> {
       const TypeMapper::TypeUpdates& mapping;
-      bool addedBlock = false;
 
       Preoptimizer(const TypeMapper::TypeUpdates& mapping) : mapping(mapping) {}
 
@@ -331,7 +329,6 @@ struct AbstractTypeRefining : public Pass {
       }
 
       Block* localizeChildren(Expression* curr) {
-        addedBlock = true;
         return ChildLocalizer(
                  curr, getFunction(), *getModule(), getPassOptions())
           .getChildrenReplacement();
@@ -420,7 +417,6 @@ struct AbstractTypeRefining : public Pass {
           // optimize to. In this case it must trap.
           replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
                                               builder.makeUnreachable()));
-          addedBlock = true;
         } else {
           // The descriptor is abstract, but it has a subtype that is not
           // (which we want to optimize to). We know this will only succeed if
@@ -490,17 +486,10 @@ struct AbstractTypeRefining : public Pass {
         if (getFunction()) {
           replaceCurrent(getDroppedChildrenAndAppend(
             curr, *getModule(), getPassOptions(), builder.makeUnreachable()));
-          addedBlock = true;
         } else {
           curr->desc = builder.makeRefNull(HeapType::none);
         }
       }
-
-      void visitFunction(Function* func) {
-        if (addedBlock) {
-          EHUtils::handleBlockNestedPops(func, *getModule());
-        }
-      }
     } preoptimizer(mapping);
     preoptimizer.run(getPassRunner(), &module);
     preoptimizer.runOnModuleCode(getPassRunner(), &module);

src/passes/CodeFolding.cpp

@@ -61,7 +61,6 @@
 
 #include "ir/branch-utils.h"
 #include "ir/effects.h"
-#include "ir/eh-utils.h"
 #include "ir/find_all.h"
 #include "ir/iteration.h"
 #include "ir/label-utils.h"
@@ -119,9 +118,6 @@ struct CodeFolding
 
   // Set when we optimized and believe another pass is warranted.
   bool anotherPass;
-  // Set when we optimized in a manner that requires EH fixups specifically,
-  // which is generally the case when we wrap things in a block.
-  bool needEHFixups;
 
   // pass state
 
@@ -286,7 +282,6 @@ struct CodeFolding
     anotherPass = true;
     while (anotherPass) {
       anotherPass = false;
-      needEHFixups = false;
       Super::doWalkFunction(func);
       optimizeTerminatingTails(unreachableTails);
       // optimize returns at the end, so we can benefit from a fallthrough if
@@ -301,9 +296,6 @@ struct CodeFolding
       unoptimizables.clear();
       modifieds.clear();
       exitingBranchCache.clear();
-      if (needEHFixups) {
-        EHUtils::handleBlockNestedPops(func, *getModule());
-      }
     }
   }
 
@@ -629,7 +621,6 @@ struct CodeFolding
     // ensure the replacement has the same type, so the outside is not surprised
     block->finalize(oldType);
     replaceCurrent(block);
-    needEHFixups = true;
   }
 
   // optimize tails that terminate control flow in this function, so we
@@ -899,7 +890,6 @@ struct CodeFolding
     // ensure the replacement has the same type, so the outside is not surprised
     outer->finalize(getFunction()->getResults());
     getFunction()->body = outer;
-    needEHFixups = true;
     return true;
   }
 

src/passes/ConstraintAnalysis.cpp

@@ -72,7 +72,6 @@
 #include "cfg/wto.h"
 #include "ir/constraint.h"
 #include "ir/drop.h"
-#include "ir/eh-utils.h"
 #include "ir/literal-utils.h"
 #include "ir/local-graph.h"
 #include "ir/properties.h"
@@ -390,7 +389,6 @@ struct ConstraintAnalysis
 
     if (refinalize) {
       ReFinalize().walkFunctionInModule(getFunction(), getModule());
-      EHUtils::handleBlockNestedPops(getFunction(), *getModule());
     }
   }
 

src/passes/DeadArgumentElimination2.cpp

@@ -44,7 +44,6 @@
 
 #include "analysis/lattices/bool.h"
 #include "ir/effects.h"
-#include "ir/eh-utils.h"
 #include "ir/intrinsics.h"
 #include "ir/label-utils.h"
 #include "ir/local-graph.h"
@@ -1113,9 +1112,6 @@ struct Optimizer
 
     assert(!hasRemovedResult || !func->body->type.isConcrete());
 
-    // We may have moved pops around. Fix them.
-    EHUtils::handleBlockNestedPops(func, *wasm);
-
     // We may have introduced a new non-nullable local whose sets do not
     // structurally dominate its gets. Fix it.
     if (hasNewNonNullableLocal) {

src/passes/DeadCodeElimination.cpp

@@ -28,7 +28,6 @@
 // have no side effects.
 //
 
-#include "ir/eh-utils.h"
 #include "ir/iteration.h"
 #include "ir/properties.h"
 #include "ir/type-updating.h"
@@ -56,10 +55,6 @@ struct DeadCodeElimination
   // as we remove code, we must keep the types of other nodes valid
   TypeUpdater typeUpdater;
 
-  // Information used to decide whether we need EH fixups at the end
-  bool hasPop = false;     // Do we have a 'pop' in this function?
-  bool addedBlock = false; // Have we added blocks in this function?
-
   Expression* replaceCurrent(Expression* expression) {
     auto* old = getCurrent();
     if (old == expression) {
@@ -77,10 +72,6 @@ struct DeadCodeElimination
   }
 
   void visitExpression(Expression* curr) {
-    if (curr->is<Pop>()) {
-      hasPop = true;
-    }
-
     if (!Properties::isControlFlowStructure(curr)) {
       // Control flow structures require special handling, but others are
       // simple.
@@ -113,7 +104,6 @@ struct DeadCodeElimination
           if (remainingChildren.size() == 1) {
             replaceCurrent(remainingChildren[0]);
           } else {
-            addedBlock = true;
             replaceCurrent(builder.makeBlock(remainingChildren));
           }
         }
@@ -195,15 +185,6 @@ struct DeadCodeElimination
       WASM_UNREACHABLE("unimplemented DCE control flow structure");
     }
   }
-
-  void visitFunction(Function* curr) {
-    // We conservatively run the EH pop fixup if this function has a 'pop' and
-    // if we have ever added blocks in the optimization, which may have moved
-    // pops into the blocks.
-    if (hasPop && addedBlock) {
-      EHUtils::handleBlockNestedPops(curr, *getModule());
-    }
-  }
 };
 
 Pass* createDeadCodeEliminationPass() { return new DeadCodeElimination(); }

src/passes/Directize.cpp

@@ -31,7 +31,6 @@
 
 #include "call-utils.h"
 #include "ir/drop.h"
-#include "ir/eh-utils.h"
 #include "ir/find_all.h"
 #include "ir/table-utils.h"
 #include "ir/utils.h"
@@ -53,8 +52,6 @@ struct FunctionDirectizer : public WalkerPass<PostWalker<FunctionDirectizer>> {
 
   FunctionDirectizer(const TableUtils::TableInfoMap& tables) : tables(tables) {}
 
-  bool optimized = false;
-
   void visitCallIndirect(CallIndirect* curr) {
     auto& table = tables.at(curr->table);
     if (!table.canOptimizeByEntry()) {
@@ -65,7 +62,6 @@ struct FunctionDirectizer : public WalkerPass<PostWalker<FunctionDirectizer>> {
       std::vector<Expression*> operands(curr->operands.begin(),
                                         curr->operands.end());
       makeDirectCall(operands, curr->target, table, curr);
-      optimized = true;
       return;
     }
 
@@ -78,25 +74,16 @@ struct FunctionDirectizer : public WalkerPass<PostWalker<FunctionDirectizer>> {
           *getFunction(),
           *getModule())) {
       replaceCurrent(calls);
-      optimized = true;
       // Note that types may have changed, as the utility here can add locals
       // which require fixups if they are non-nullable, for example.
       changedTypes = true;
       return;
     }
   }
 
-  bool hasTry = false;
-
-  void visitTry(Try* curr) { hasTry = true; }
-
   void doWalkFunction(Function* func) {
     WalkerPass<PostWalker<FunctionDirectizer>>::doWalkFunction(func);
 
-    if (optimized && hasTry) {
-      EHUtils::handleBlockNestedPops(func, *getModule());
-    }
-
     if (changedTypes) {
       ReFinalize().walkFunctionInModule(func, getModule());
     }

src/passes/Flatten.cpp

@@ -39,7 +39,6 @@
 
 #include <ir/branch-utils.h>
 #include <ir/effects.h>
-#include <ir/eh-utils.h>
 #include <ir/flat.h>
 #include <ir/properties.h>
 #include <ir/utils.h>
@@ -376,10 +375,6 @@ struct Flatten
     }
     // the body may have preludes
     curr->body = getPreludesWithExpression(originalBody, curr->body);
-
-    // Flatten can generate blocks within 'catch', making pops invalid. Fix them
-    // up.
-    EHUtils::handleBlockNestedPops(curr, *getModule());
   }
 
 private:

src/passes/GUFA.cpp

@@ -44,7 +44,6 @@
 //
 
 #include "ir/drop.h"
-#include "ir/eh-utils.h"
 #include "ir/possible-contents.h"
 #include "ir/properties.h"
 #include "ir/utils.h"
@@ -318,9 +317,6 @@ struct GUFAOptimizer
       return;
     }
 
-    // We may add blocks around pops, which we must fix up.
-    EHUtils::handleBlockNestedPops(func, *getModule());
-
     // If we are in "optimizing" mode, we'll also run some more passes on this
     // function that we just optimized. If not, leave now.
     if (!optimizing) {