Merge pull request #710 from WebAssembly/autodrop-fix

Autodrop fixes

4 files changed, 99 insertions, 55 deletions

diff --git a/src/asm2wasm.h b/src/asm2wasm.h
index 1c528d7f3..e926e381f 100644
--- a/src/asm2wasm.h
+++ b/src/asm2wasm.h
@@ -797,13 +797,17 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
           switch (curr->type) {
             case i32: replaceCurrent(parent->builder.makeUnary(TruncSFloat64ToInt32, curr)); break;
             case f32: replaceCurrent(parent->builder.makeUnary(DemoteFloat64, curr)); break;
-            case none: replaceCurrent(parent->builder.makeDrop(curr)); break;
+            case none: {
+              // this function returns a value, but we are not using it, so it must be dropped.
+              // autodrop will do that for us.
+              break;
+            }
             default: WASM_UNREACHABLE();
           }
         } else {
           assert(curr->type == none);
           // we don't want a return value here, but the import does provide one
-          replaceCurrent(parent->builder.makeDrop(curr));
+          // autodrop will do that for us.
         }
         curr->type = importResult;
       }
diff --git a/src/ast_utils.h b/src/ast_utils.h
index ff5d93e02..9741faa25 100644
--- a/src/ast_utils.h
+++ b/src/ast_utils.h
@@ -422,6 +422,32 @@ struct ExpressionAnalyzer {
     return func->result != none;
   }
 
+  // Checks if a value is dropped.
+  static bool isResultDropped(std::vector<Expression*> stack) {
+    for (int i = int(stack.size()) - 2; i >= 0; i--) {
+      auto* curr = stack[i];
+      auto* above = stack[i + 1];
+      if (curr->is<Block>()) {
+        auto* block = curr->cast<Block>();
+        for (size_t j = 0; j < block->list.size() - 1; j++) {
+          if (block->list[j] == above) return false;
+        }
+        assert(block->list.back() == above);
+        // continue down
+      } else if (curr->is<If>()) {
+        auto* iff = curr->cast<If>();
+        if (above == iff->condition) return false;
+        if (!iff->ifFalse) return false;
+        assert(above == iff->ifTrue || above == iff->ifFalse);
+        // continue down
+      } else {
+        if (curr->is<Drop>()) return true; // dropped
+        return false; // all other node types use the result
+      }
+    }
+    return false;
+  }
+
   // Checks if a break is a simple - no condition, no value, just a plain branching
   static bool isSimple(Break* curr) {
     return !curr->condition && !curr->value;
@@ -856,6 +882,34 @@ struct ExpressionAnalyzer {
   }
 };
 
+// Finalizes a node
+
+struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, Visitor<ReFinalize>>> {
+  void visitBlock(Block *curr) { curr->finalize(); }
+  void visitIf(If *curr) { curr->finalize(); }
+  void visitLoop(Loop *curr) { curr->finalize(); }
+  void visitBreak(Break *curr) { curr->finalize(); }
+  void visitSwitch(Switch *curr) { curr->finalize(); }
+  void visitCall(Call *curr) { curr->finalize(); }
+  void visitCallImport(CallImport *curr) { curr->finalize(); }
+  void visitCallIndirect(CallIndirect *curr) { curr->finalize(); }
+  void visitGetLocal(GetLocal *curr) { curr->finalize(); }
+  void visitSetLocal(SetLocal *curr) { curr->finalize(); }
+  void visitGetGlobal(GetGlobal *curr) { curr->finalize(); }
+  void visitSetGlobal(SetGlobal *curr) { curr->finalize(); }
+  void visitLoad(Load *curr) { curr->finalize(); }
+  void visitStore(Store *curr) { curr->finalize(); }
+  void visitConst(Const *curr) { curr->finalize(); }
+  void visitUnary(Unary *curr) { curr->finalize(); }
+  void visitBinary(Binary *curr) { curr->finalize(); }
+  void visitSelect(Select *curr) { curr->finalize(); }
+  void visitDrop(Drop *curr) { curr->finalize(); }
+  void visitReturn(Return *curr) { curr->finalize(); }
+  void visitHost(Host *curr) { curr->finalize(); }
+  void visitNop(Nop *curr) { curr->finalize(); }
+  void visitUnreachable(Unreachable *curr) { curr->finalize(); }
+};
+
 // Adds drop() operations where necessary. This lets you not worry about adding drop when
 // generating code.
 struct AutoDrop : public WalkerPass<ExpressionStackWalker<AutoDrop, Visitor<AutoDrop>>> {
@@ -863,6 +917,26 @@ struct AutoDrop : public WalkerPass<ExpressionStackWalker<AutoDrop, Visitor<Auto
 
   Pass* create() override { return new AutoDrop; }
 
+  bool maybeDrop(Expression*& child) {
+    bool acted = false;
+    if (isConcreteWasmType(child->type)) {
+      expressionStack.push_back(child);
+      if (!ExpressionAnalyzer::isResultUsed(expressionStack, getFunction()) && !ExpressionAnalyzer::isResultDropped(expressionStack)) {
+        child = Builder(*getModule()).makeDrop(child);
+        acted = true;
+      }
+      expressionStack.pop_back();
+    }
+    return acted;
+  }
+
+  void reFinalize() {
+    for (int i = int(expressionStack.size()) - 1; i >= 0; i--) {
+      auto* curr = expressionStack[i];
+      ReFinalize().visit(curr);
+    }
+  }
+
   void visitBlock(Block* curr) {
     if (curr->list.size() == 0) return;
     for (Index i = 0; i < curr->list.size() - 1; i++) {
@@ -871,31 +945,21 @@ struct AutoDrop : public WalkerPass<ExpressionStackWalker<AutoDrop, Visitor<Auto
         curr->list[i] = Builder(*getModule()).makeDrop(child);
       }
     }
-    auto* last = curr->list.back();
-    expressionStack.push_back(last);
-    if (isConcreteWasmType(last->type) && !ExpressionAnalyzer::isResultUsed(expressionStack, getFunction())) {
-      curr->list.back() = Builder(*getModule()).makeDrop(last);
+    if (maybeDrop(curr->list.back())) {
+      reFinalize();
+      assert(curr->type == none);
     }
-    expressionStack.pop_back();
-    curr->finalize(); // we may have changed our type
   }
 
   void visitIf(If* curr) {
+    bool acted = false;
+    if (maybeDrop(curr->ifTrue)) acted = true;
     if (curr->ifFalse) {
-      if (!isConcreteWasmType(curr->type)) {
-        // if either side of an if-else not returning a value is concrete, drop it
-        if (isConcreteWasmType(curr->ifTrue->type)) {
-          curr->ifTrue = Builder(*getModule()).makeDrop(curr->ifTrue);
-        }
-        if (isConcreteWasmType(curr->ifFalse->type)) {
-          curr->ifFalse = Builder(*getModule()).makeDrop(curr->ifFalse);
-        }
-      }
-    } else {
-      // if without else does not return a value, so the body must be dropped if it is concrete
-      if (isConcreteWasmType(curr->ifTrue->type)) {
-        curr->ifTrue = Builder(*getModule()).makeDrop(curr->ifTrue);
-      }
+      if (maybeDrop(curr->ifFalse)) acted = true;
+    }
+    if (acted) {
+      reFinalize();
+      assert(curr->type == none);
     }
   }
 
@@ -906,34 +970,6 @@ struct AutoDrop : public WalkerPass<ExpressionStackWalker<AutoDrop, Visitor<Auto
   }
 };
 
-// Finalizes a node
-
-struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, Visitor<ReFinalize>>> {
-  void visitBlock(Block *curr) { curr->finalize(); }
-  void visitIf(If *curr) { curr->finalize(); }
-  void visitLoop(Loop *curr) { curr->finalize(); }
-  void visitBreak(Break *curr) { curr->finalize(); }
-  void visitSwitch(Switch *curr) { curr->finalize(); }
-  void visitCall(Call *curr) { curr->finalize(); }
-  void visitCallImport(CallImport *curr) { curr->finalize(); }
-  void visitCallIndirect(CallIndirect *curr) { curr->finalize(); }
-  void visitGetLocal(GetLocal *curr) { curr->finalize(); }
-  void visitSetLocal(SetLocal *curr) { curr->finalize(); }
-  void visitGetGlobal(GetGlobal *curr) { curr->finalize(); }
-  void visitSetGlobal(SetGlobal *curr) { curr->finalize(); }
-  void visitLoad(Load *curr) { curr->finalize(); }
-  void visitStore(Store *curr) { curr->finalize(); }
-  void visitConst(Const *curr) { curr->finalize(); }
-  void visitUnary(Unary *curr) { curr->finalize(); }
-  void visitBinary(Binary *curr) { curr->finalize(); }
-  void visitSelect(Select *curr) { curr->finalize(); }
-  void visitDrop(Drop *curr) { curr->finalize(); }
-  void visitReturn(Return *curr) { curr->finalize(); }
-  void visitHost(Host *curr) { curr->finalize(); }
-  void visitNop(Nop *curr) { curr->finalize(); }
-  void visitUnreachable(Unreachable *curr) { curr->finalize(); }
-};
-
 } // namespace wasm
 
 #endif // wasm_ast_utils_h
diff --git a/src/passes/SimplifyLocals.cpp b/src/passes/SimplifyLocals.cpp
index e032acc77..acc6860be 100644
--- a/src/passes/SimplifyLocals.cpp
+++ b/src/passes/SimplifyLocals.cpp
@@ -351,9 +351,8 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals,
   // optimize set_locals from both sides of an if into a return value
   void optimizeIfReturn(If* iff, Expression** currp, Sinkables& ifTrue) {
     assert(iff->ifFalse);
-    // if this if already has a result that is used, we can't do anything
-    assert(expressionStack.back() == iff);
-    if (ExpressionAnalyzer::isResultUsed(expressionStack, getFunction())) return;
+    // if this if already has a result, we can't do anything
+    if (isConcreteWasmType(iff->type)) return;
     // We now have the sinkables from both sides of the if.
     Sinkables& ifFalse = sinkables;
     Index sharedIndex = -1;
diff --git a/src/passes/Vacuum.cpp b/src/passes/Vacuum.cpp
index 0fdd12dec..03becb04c 100644
--- a/src/passes/Vacuum.cpp
+++ b/src/passes/Vacuum.cpp
@@ -193,9 +193,14 @@ struct Vacuum : public WalkerPass<ExpressionStackWalker<Vacuum, Visitor<Vacuum>>
         curr->ifTrue = curr->ifFalse;
         curr->ifFalse = nullptr;
         curr->condition = Builder(*getModule()).makeUnary(EqZInt32, curr->condition);
+      } else if (curr->ifTrue->is<Drop>() && curr->ifFalse->is<Drop>()) {
+        // instead of dropping both sides, drop the if
+        curr->ifTrue = curr->ifTrue->cast<Drop>()->value;
+        curr->ifFalse = curr->ifFalse->cast<Drop>()->value;
+        curr->finalize();
+        replaceCurrent(Builder(*getModule()).makeDrop(curr));
       }
-    }
-    if (!curr->ifFalse) {
+    } else {
       // no else
       if (curr->ifTrue->is<Nop>()) {
         // no nothing