Add fixup function for nested pops in catch

This adds `EHUtils::handleBlockNestedPops`, which can be called at the
end of passes that has a possibility to put `pop`s inside `block`s. This
method assumes there exists a `pop` in a first-descendant line, even
though it can be nested within a block. This allows a `pop` to be nested
within a `block` or a `try`, but not a `loop`, since that means the
`pop` can run multile times. In case of `if`, `pop` can exist only in
its condition; if a `pop` is in its true or false body, that's not in
the first-descendant line.

To test this, this adds `passes/test_passes.cpp`, which is intended to
contain multiple test passes that test a single (or more) utility
functions separately. Without this kind of pass, it is hard to test
various cases in which nested `pop`s can be generated in existing
passes. This PR also adds `PassRegistry::registerTestPass`, which
registers a pass that's intended only for internal testing and does not
show up in `wasm-opt --help`.

Author: aheejin

src/ir/eh-utils.cpp

@@ -16,53 +16,137 @@
 
 #include "ir/eh-utils.h"
 #include "ir/branch-utils.h"
+#include "ir/find_all.h"
 
 namespace wasm {
 
 namespace EHUtils {
 
-bool isPopValid(Expression* catchBody) {
-  Expression* firstChild = nullptr;
-  auto* block = catchBody->dynCast<Block>();
-  if (!block) {
-    firstChild = catchBody;
-  } else {
-    // When there are multiple expressions within a catch body, an implicit
-    // block is created within it for convenience purposes, and if there are no
-    // branches that targets the block, it will be omitted when written back.
-    // But if there is a branch targetting this block, this block cannot be
-    // removed, and 'pop''s location will be like
-    // (catch $e
-    //   (block $l0
-    //     (pop i32) ;; within a block!
-    //     (br $l0)
-    //     ...
-    //   )
-    // )
-    // which is invalid.
-    if (BranchUtils::BranchSeeker::has(block, block->name)) {
-      return false;
-    }
-    // There should be a pop somewhere
-    if (block->list.empty()) {
-      return false;
-    }
-    firstChild = *block->list.begin();
-  }
+// This returns three values, some of them as output parameters:
+// - Return value: 'pop' expression (Expression*), when there is one in
+//   first-descendant line. If there's no such pop, it returns null.
+// - isPopNested: Whether the discovered 'pop' is nested within a block
+// - popPtr: 'pop' expression's pointer (Expression**), when there is one found
+//
+// When 'catchBody' itself is a 'pop', 'pop''s pointer is null, because there is
+// no way to get the given expression's address. But that's fine because pop's
+// pointer is only necessary (in handleBlockNestedPops) to fix it up when it is
+// nested, and if 'catchBody' itself is a pop, we don't need to fix it up.
+static Expression*
+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-decendent line.
+  // in that first-decendant line.
+  Expression** firstChildPtr = nullptr;
   while (true) {
     if (firstChild->is<Pop>()) {
-      return true;
+      popPtr = firstChildPtr;
+      return firstChild;
     }
-    // We use ValueChildIterator in order not to go into block/loop/try/if
-    // bodies, because a pop cannot be in those control flow expressions.
-    ValueChildIterator it(firstChild);
+
+    if (Properties::isControlFlowStructure(firstChild)) {
+      // If's condition is a value child who comes before an 'if' instruction
+      // in binary, it is fine if a 'pop' is in there. We don't allow a 'pop' to
+      // be in an 'if''s then or else body because they are not
+      // first descendants.
+      if (auto* if_ = firstChild->dynCast<If>()) {
+        firstChild = if_->condition;
+        continue;
+      }
+      // We don't allow the pop to be included in a loop, because it cannot be
+      // run more than once
+      if (firstChild->is<Loop>()) {
+        return nullptr;
+      }
+      if (firstChild->is<Block>()) {
+        // If there are no branches that targets the implicit block, it will be
+        // removed when written back. But there are branches that target the
+        // implicit block,
+        // (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 {
+          isPopNested = true;
+        }
+      }
+      if (firstChild->is<Try>()) {
+        isPopNested = true;
+      }
+    }
+    ChildIterator it(firstChild);
     if (it.begin() == it.end()) {
-      return false;
+      return nullptr;
+    }
+    firstChildPtr = &*it.begin();
+    firstChild = *firstChildPtr;
+  }
+}
+
+bool isPopValid(Expression* catchBody) {
+  bool isPopNested = false;
+  Expression** popPtr = nullptr;
+  auto* pop = getFirstPop(catchBody, isPopNested, popPtr);
+  return pop != nullptr && !isPopNested;
+}
+
+void handleBlockNestedPops(Function* func, Module& wasm) {
+  Builder builder(wasm);
+  FindAll<Try> trys(func->body);
+  for (auto* try_ : trys.list) {
+    for (Index i = 0; i < try_->catchTags.size(); i++) {
+      Name tagName = try_->catchTags[i];
+      auto* tag = wasm.getTagOrNull(tagName);
+      if (tag->sig.params == Type::none) {
+        continue;
+      }
+
+      auto* catchBody = try_->catchBodies[i];
+      bool isPopNested = false;
+      Expression** popPtr = nullptr;
+      Expression* pop = getFirstPop(catchBody, isPopNested, popPtr);
+      assert(pop && "Pop has not been found in this catch");
+
+      // Change code like
+      // (catch $e
+      //   ...
+      //   (block
+      //     (pop i32)
+      //   )
+      // )
+      // into
+      // (catch $e
+      //   (local.set $new
+      //     (pop i32)
+      //   )
+      //   ...
+      //   (block
+      //     (local.get $new)
+      //   )
+      // )
+      if (isPopNested) {
+        assert(popPtr);
+        Index newLocal = builder.addVar(func, pop->type);
+        try_->catchBodies[i] =
+          builder.makeSequence(builder.makeLocalSet(newLocal, pop), catchBody);
+        *popPtr = builder.makeLocalGet(newLocal, pop->type);
+      }
     }
-    firstChild = *it.begin();
   }
 }
 

src/ir/eh-utils.h

@@ -20,6 +20,8 @@
 namespace wasm {
 
 class Expression;
+class Function;
+class Module;
 
 namespace EHUtils {
 
@@ -31,6 +33,12 @@ namespace EHUtils {
 //   catch body, which is invalid. That will be taken care of in validation.
 bool isPopValid(Expression* catchBody);
 
+// Fixes up 'pop's nested in blocks, which are currently not supported without
+// block param types, by creating a new local, putting a (local.set $new (pop
+// type)) right after 'catch', and putting a '(local.get $new)' where the 'pop'
+// used to be.
+void handleBlockNestedPops(Function* func, Module& wasm);
+
 } // namespace EHUtils
 
 } // namespace wasm

src/pass.h

@@ -39,8 +39,12 @@ struct PassRegistry {
   typedef std::function<Pass*()> Creator;
 
   void registerPass(const char* name, const char* description, Creator create);
+  // Register a pass that's used for internal testing. These pass do not show up
+  // in --help.
+  void
+  registerTestPass(const char* name, const char* description, Creator create);
   std::unique_ptr<Pass> createPass(std::string name);
-  std::vector<std::string> getRegisteredNames();
+  std::vector<std::string> getRegisteredNames(bool includeHidden = false);
   std::string getPassDescription(std::string name);
 
 private:
@@ -49,9 +53,10 @@ struct PassRegistry {
   struct PassInfo {
     std::string description;
     Creator create;
+    bool hidden;
     PassInfo() = default;
-    PassInfo(std::string description, Creator create)
-      : description(description), create(create) {}
+    PassInfo(std::string description, Creator create, bool hidden = false)
+      : description(description), create(create), hidden(false) {}
   };
   std::map<std::string, PassInfo> passInfos;
 };

src/passes/CMakeLists.txt

@@ -9,6 +9,7 @@ add_custom_command(
 FILE(GLOB passes_HEADERS *.h)
 set(passes_SOURCES
   pass.cpp
+  test_passes.cpp
   AlignmentLowering.cpp
   Asyncify.cpp
   AvoidReinterprets.cpp

src/passes/pass.cpp

@@ -47,6 +47,13 @@ void PassRegistry::registerPass(const char* name,
   passInfos[name] = PassInfo(description, create);
 }
 
+void PassRegistry::registerTestPass(const char* name,
+                                    const char* description,
+                                    Creator create) {
+  assert(passInfos.find(name) == passInfos.end());
+  passInfos[name] = PassInfo(description, create, true);
+}
+
 std::unique_ptr<Pass> PassRegistry::createPass(std::string name) {
   if (passInfos.find(name) == passInfos.end()) {
     Fatal() << "Could not find pass: " << name << "\n";
@@ -57,10 +64,11 @@ std::unique_ptr<Pass> PassRegistry::createPass(std::string name) {
   return ret;
 }
 
-std::vector<std::string> PassRegistry::getRegisteredNames() {
+std::vector<std::string> PassRegistry::getRegisteredNames(bool includeHidden) {
   std::vector<std::string> ret;
   for (auto pair : passInfos) {
-    ret.push_back(pair.first);
+    if (includeHidden || !pair.second.hidden)
+      ret.push_back(pair.first);
   }
   return ret;
 }
@@ -407,6 +415,11 @@ void PassRegistry::registerPasses() {
   registerPass("vacuum", "removes obviously unneeded code", createVacuumPass);
   // registerPass(
   //   "lower-i64", "lowers i64 into pairs of i32s", createLowerInt64Pass);
+
+  // Register passes used for internal testing. These don't show up in --help.
+  registerTestPass("catch-pop-fixup",
+                   "fixup nested pops within catches",
+                   createCatchPopFixupPass);
 }
 
 void PassRunner::addIfNoDWARFIssues(std::string passName) {

src/passes/passes.h

@@ -21,7 +21,7 @@ namespace wasm {
 
 class Pass;
 
-// All passes:
+// Normal passes:
 Pass* createAlignmentLoweringPass();
 Pass* createAsyncifyPass();
 Pass* createAvoidReinterpretsPass();
@@ -135,6 +135,9 @@ Pass* createTypeRefiningPass();
 Pass* createUnteePass();
 Pass* createVacuumPass();
 
+// Test passes:
+Pass* createCatchPopFixupPass();
+
 } // namespace wasm
 
 #endif

src/passes/test_passes.cpp

@@ -0,0 +1,46 @@
+/*
+ * Copyright 2021 WebAssembly Community Group participants
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+//
+// This file contains passes used for internal testing. This file can be used to
+// test utility functions separately.
+//
+
+#include "ir/eh-utils.h"
+#include "pass.h"
+#include "wasm-builder.h"
+#include "wasm.h"
+
+namespace wasm {
+
+namespace {
+
+// Pass to test EHUtil::handleBlockNestedPops function
+struct CatchPopFixup : public WalkerPass<PostWalker<CatchPopFixup>> {
+  bool isFunctionParallel() override { return true; }
+  Pass* create() override { return new CatchPopFixup; }
+
+  void doWalkFunction(Function* func) {
+    EHUtils::handleBlockNestedPops(func, *getModule());
+  }
+  void run(PassRunner* runner, Module* module) override {}
+};
+
+} // anonymous namespace
+
+Pass* createCatchPopFixupPass() { return new CatchPopFixup(); }
+
+} // namespace wasm