Reset function context when ending a function in IRBuilder

src/passes/J2CLOpts.cpp

@@ -43,6 +43,7 @@ Expression* getTrivialFunctionBody(Function* func) {
   // Only consider trivial the following instructions which can be safely
   // inlined and note that their size is at most 2.
   if (body->is<Nop>() || body->is<GlobalGet>() || body->is<Const>() ||
+      (body->is<Block>() && body->cast<Block>()->list.empty()) ||
       // Call with no arguments.
       (body->is<Call>() && body->dynCast<Call>()->operands.size() == 0) ||
       // Simple global.set with a constant.

src/passes/Outlining.cpp

@@ -42,8 +42,8 @@ namespace wasm {
 struct ReconstructStringifyWalker
   : public StringifyWalker<ReconstructStringifyWalker> {
 
-  ReconstructStringifyWalker(Module* wasm)
-    : existingBuilder(*wasm), outlinedBuilder(*wasm) {
+  ReconstructStringifyWalker(Module* wasm, Function* func)
+    : existingBuilder(*wasm), outlinedBuilder(*wasm), func(func) {
     this->setModule(wasm);
     DBG(std::cerr << "\nexistingBuilder: " << &existingBuilder
                   << " outlinedBuilder: " << &outlinedBuilder << "\n");
@@ -77,6 +77,9 @@ struct ReconstructStringifyWalker
   // contain repeat sequences found in the program.
   IRBuilder outlinedBuilder;
 
+  // The function we are outlining from.
+  Function* func;
+
   void addUniqueSymbol(SeparatorReason reason) {
     if (auto curr = reason.getFuncStart()) {
       startExistingFunction(curr->func);
@@ -108,6 +111,8 @@ struct ReconstructStringifyWalker
       DBG(desc = "Loop Start at ");
     } else if (reason.getEnd()) {
       ASSERT_OK(existingBuilder.visitEnd());
+      // Reset the function in case we just ended the function scope.
+      existingBuilder.setFunction(func);
       // Outlining performs an unnested walk of the Wasm module, visiting
       // each scope one at a time. IRBuilder, in contrast, expects to
       // visit several nested scopes at a time. Thus, calling end() finalizes
@@ -346,15 +351,16 @@ struct Outlining : public Pass {
 
   void outline(Module* module, Sequences seqByFunc) {
     // TODO: Make this a function-parallel sub-pass.
-    ReconstructStringifyWalker reconstruct(module);
     std::vector<Name> keys(seqByFunc.size());
     std::transform(seqByFunc.begin(),
                    seqByFunc.end(),
                    keys.begin(),
                    [](auto pair) { return pair.first; });
-    for (auto func : keys) {
-      reconstruct.sequences = std::move(seqByFunc[func]);
-      reconstruct.doWalkFunction(module->getFunction(func));
+    for (auto funcName : keys) {
+      auto* func = module->getFunction(funcName);
+      ReconstructStringifyWalker reconstruct(module, func);
+      reconstruct.sequences = std::move(seqByFunc[funcName]);
+      reconstruct.doWalkFunction(func);
     }
   }
 

src/wasm-ir-builder.h

@@ -40,8 +40,7 @@ namespace wasm {
 // globals, tables, functions, etc.) to already exist in the module.
 class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
 public:
-  IRBuilder(Module& wasm, Function* func = nullptr)
-    : wasm(wasm), func(func), builder(wasm) {}
+  IRBuilder(Module& wasm) : wasm(wasm), builder(wasm) {}
 
   // Get the valid Binaryen IR expression representing the sequence of visited
   // instructions. The IRBuilder is reset and can be used with a fresh sequence
@@ -60,6 +59,10 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
   // pushed instruction.
   void setDebugLocation(const std::optional<Function::DebugLocation>&);
 
+  // Set the function used to add scratch locals when constructing an isolated
+  // sequence of IR.
+  void setFunction(Function* func) { this->func = func; }
+
   // Handle the boundaries of control flow structures. Users may choose to use
   // the corresponding `makeXYZ` function below instead of `visitXYZStart`, but
   // either way must call `visitEnd` and friends at the appropriate times.
@@ -234,7 +237,7 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
 
 private:
   Module& wasm;
-  Function* func;
+  Function* func = nullptr;
   Builder builder;
 
   // The location lacks debug info as it was marked as not having it.

src/wasm/wasm-ir-builder.cpp

@@ -157,7 +157,7 @@ void IRBuilder::push(Expression* expr) {
 
 Result<Expression*> IRBuilder::build() {
   if (scopeStack.empty()) {
-    return builder.makeNop();
+    return builder.makeBlock();
   }
   if (scopeStack.size() > 1 || !scopeStack.back().isNone()) {
     return Err{"unfinished block context"};
@@ -792,12 +792,13 @@ Result<Expression*> IRBuilder::finishScope(Block* block) {
   Expression* ret = nullptr;
   if (scope.exprStack.size() == 0) {
     // No expressions for this scope, but we need something. If we were given a
-    // block, we can empty it out and return it, but otherwise we need a nop.
+    // block, we can empty it out and return it, but otherwise create a new
+    // empty block.
     if (block) {
       block->list.clear();
       ret = block;
     } else {
-      ret = builder.makeNop();
+      ret = builder.makeBlock();
     }
   } else if (scope.exprStack.size() == 1) {
     // We can put our single expression directly into the surrounding scope.
@@ -980,6 +981,7 @@ Result<> IRBuilder::visitEnd() {
     if (scope.needsPopFixup()) {
       EHUtils::handleBlockNestedPops(func, wasm);
     }
+    this->func = nullptr;
   } else if (auto* block = scope.getBlock()) {
     assert(*expr == block);
     block->name = scope.label;

test/binaryen.js/debug-info.js.txt

@@ -5,7 +5,6 @@ debugInfo=false
  (memory $0 0)
  (export "test" (func $0))
  (func $0
-  (nop)
  )
 )
 
@@ -16,7 +15,6 @@ debugInfo=true
  (memory $0 0)
  (export "test" (func $test))
  (func $test
-  (nop)
  )
 )
 
@@ -27,7 +25,6 @@ debugInfo=false
  (memory $0 0)
  (export "test" (func $0))
  (func $0
-  (nop)
  )
 )
 

test/binaryen.js/debug-names.js.txt

@@ -16,7 +16,6 @@
  (table $wor 0 0 funcref)
  (func $of (param $wasm i32)
   (local $!#$%&'*+-./:<=>?@\^_`|~ f64)
-  (nop)
  )
 )
 
@@ -28,7 +27,6 @@
  (table $wor 0 0 funcref)
  (func $of (param $js i32)
   (local $!#$%&'*+-./:<=>?@\5c^_`|~ f64)
-  (nop)
  )
 )
 
@@ -40,7 +38,6 @@
  (table $wor 0 0 funcref)
  (func $of (param $js i32)
   (local $!#$%&'*+-./:<=>?@\5c^_`|~ f64)
-  (nop)
  )
 )
 

test/example/module-splitting.txt

@@ -585,7 +585,6 @@ Before:
  (elem $0 (global.get $base) $null $foo)
  (export "foo" (func $foo))
  (func $null (type $0)
-  (nop)
  )
  (func $foo (type $1) (param $0 i32) (result i32)
   (local.get $0)
@@ -607,7 +606,6 @@ After:
  (export "%table_2" (table $0))
  (export "%global" (global $base))
  (func $null (type $0)
-  (nop)
  )
  (func $foo (type $1) (param $0 i32) (result i32)
   (call_indirect $0 (type $1)
@@ -1144,7 +1142,6 @@ Before:
  (export "foo1" (func $foo))
  (export "foo2" (func $foo))
  (func $foo (type $0)
-  (nop)
  )
 )
 Keeping: <none>
@@ -1169,7 +1166,6 @@ Secondary:
  (import "primary" "%table" (table $0 1 funcref))
  (elem $0 (i32.const 0) $foo)
  (func $foo (type $0)
-  (nop)
  )
 )
 

test/gtest/cfg.cpp

@@ -149,7 +149,7 @@ TEST_F(CFGTest, Empty) {
 ;; entry
 ;; exit
 0:
-  0: nop
+  0: block
 )cfg";
 
   Module wasm;

test/lit/basic/complexTextNames.wast

@@ -13,12 +13,10 @@
   ;; CHECK-TEXT:      (type $0 (func))
 
   ;; CHECK-TEXT:      (func $foo\20\28.bar\29 (type $0)
-  ;; CHECK-TEXT-NEXT:  (nop)
   ;; CHECK-TEXT-NEXT: )
   ;; CHECK-BIN:      (type $0 (func))
 
   ;; CHECK-BIN:      (func $foo\20\28.bar\29 (type $0)
-  ;; CHECK-BIN-NEXT:  (nop)
   ;; CHECK-BIN-NEXT: )
   (func $foo\20\28.bar\29)
 
@@ -34,7 +32,6 @@
 ;; CHECK-BIN-NODEBUG:      (type $0 (func))
 
 ;; CHECK-BIN-NODEBUG:      (func $0 (type $0)
-;; CHECK-BIN-NODEBUG-NEXT:  (nop)
 ;; CHECK-BIN-NODEBUG-NEXT: )
 
 ;; CHECK-BIN-NODEBUG:      (func $1 (type $0)

test/lit/basic/duplicate_types.wast

@@ -25,12 +25,10 @@
  ;; CHECK-TEXT:      (type $1 (func (param i32) (result i32)))
 
  ;; CHECK-TEXT:      (func $f0 (type $0) (param $0 i32)
- ;; CHECK-TEXT-NEXT:  (nop)
  ;; CHECK-TEXT-NEXT: )
  ;; CHECK-BIN:      (type $1 (func (param i32) (result i32)))
 
  ;; CHECK-BIN:      (func $f0 (type $0) (param $0 i32)
- ;; CHECK-BIN-NEXT:  (nop)
  ;; CHECK-BIN-NEXT: )
  (func $f0 (param i32))
 
@@ -47,7 +45,6 @@
 ;; CHECK-BIN-NODEBUG:      (type $1 (func (param i32) (result i32)))
 
 ;; CHECK-BIN-NODEBUG:      (func $0 (type $0) (param $0 i32)
-;; CHECK-BIN-NODEBUG-NEXT:  (nop)
 ;; CHECK-BIN-NODEBUG-NEXT: )
 
 ;; CHECK-BIN-NODEBUG:      (func $1 (type $1) (param $0 i32) (result i32)