wasm2js: unreachability fixes (#2037)

Also test in pass-debug mode, for better coverage.

1 files changed, 19 insertions, 25 deletions

diff --git a/src/passes/I64ToI32Lowering.cpp b/src/passes/I64ToI32Lowering.cpp
index e6dc90607..f16003745 100644
--- a/src/passes/I64ToI32Lowering.cpp
+++ b/src/passes/I64ToI32Lowering.cpp
@@ -223,6 +223,7 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
 
   void visitBlock(Block* curr) {
     if (curr->list.size() == 0) return;
+    if (handleUnreachable(curr)) return;
     if (curr->type == i64) curr->type = i32;
     auto highBitsIt = labelHighBitVars.find(curr->name);
     if (!hasOutParam(curr->list.back())) {
@@ -272,6 +273,7 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
   }
 
   void visitLoop(Loop* curr) {
+    // TODO: in flat IR, no chance for an out param
     assert(labelHighBitVars.find(curr->name) == labelHighBitVars.end());
     if (curr->type != i64) return;
     curr->type = i32;
@@ -279,6 +281,7 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
   }
 
   void visitBreak(Break* curr) {
+    // TODO: in flat IR, no chance for an out param
     if (!hasOutParam(curr->value)) return;
     assert(curr->value != nullptr);
     TempVar valHighBits = fetchOutParam(curr->value);
@@ -301,6 +304,7 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
   }
 
   void visitSwitch(Switch* curr) {
+    // TODO: in flat IR, no chance for an out param
     if (!hasOutParam(curr->value)) return;
     TempVar outParam = fetchOutParam(curr->value);
     TempVar tmp = getTemp();
@@ -478,8 +482,8 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
   }
 
   void visitSetGlobal(SetGlobal* curr) {
-    if (handleUnreachable(curr)) return;
     if (!originallyI64Globals.count(curr->name)) return;
+    if (handleUnreachable(curr)) return;
     TempVar highBits = fetchOutParam(curr->value);
     auto* setHigh = builder->makeSetGlobal(
       makeHighName(curr->name),
@@ -550,6 +554,7 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
       TempVar ptrTemp = getTemp();
       SetLocal* setPtr = builder->makeSetLocal(ptrTemp, curr->ptr);
       curr->ptr = builder->makeGetLocal(ptrTemp, i32);
+      curr->finalize();
       Store* storeHigh = builder->makeStore(
         4,
         curr->offset + 4,
@@ -941,11 +946,7 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
 
   void visitUnary(Unary* curr) {
     if (!unaryNeedsLowering(curr->op)) return;
-    if (curr->type == unreachable || curr->value->type == unreachable) {
-      assert(!hasOutParam(curr->value));
-      replaceCurrent(curr->value);
-      return;
-    }
+    if (handleUnreachable(curr)) return;
     assert(hasOutParam(curr->value) || curr->type == i64 || curr->type == f64);
     switch (curr->op) {
       case ClzInt64:
@@ -1463,25 +1464,8 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
   }
 
   void visitBinary(Binary* curr) {
+    if (handleUnreachable(curr)) return;
     if (!binaryNeedsLowering(curr->op)) return;
-    if (!hasOutParam(curr->left)) {
-      // left unreachable, replace self with left
-      replaceCurrent(curr->left);
-      if (hasOutParam(curr->right)) {
-        // free temp var
-        fetchOutParam(curr->right);
-      }
-      return;
-    }
-    if (!hasOutParam(curr->right)) {
-      // right unreachable, replace self with left then right
-      replaceCurrent(
-        builder->blockify(builder->makeDrop(curr->left), curr->right)
-      );
-      // free temp var
-      fetchOutParam(curr->left);
-      return;
-    }
     // left and right reachable, lower normally
     TempVar leftLow = getTemp();
     TempVar leftHigh = fetchOutParam(curr->left);
@@ -1668,16 +1652,26 @@ private:
 
   // If e.g. a select is unreachable, then one arm may have an out param
   // but not the other. In this case dce should really have been run
-  // before; handle it in a simple way here.
+  // before; handle it in a simple way here by replacing the node with
+  // a block of its children.
+  // This is valid only for nodes that execute their children
+  // unconditionally before themselves, so it is not valid for an if,
+  // in particular.
   bool handleUnreachable(Expression* curr) {
     if (curr->type != unreachable) return false;
     std::vector<Expression*> children;
+    bool hasUnreachable = false;
     for (auto* child : ChildIterator(curr)) {
       if (isConcreteType(child->type)) {
         child = builder->makeDrop(child);
+      } else if (child->type == unreachable) {
+        hasUnreachable = true;
       }
       children.push_back(child);
     }
+    if (!hasUnreachable) return false;
+    // This has an unreachable child, so we can replace it with
+    // the children.
     auto* block = builder->makeBlock(children);
     assert(block->type == unreachable);
     replaceCurrent(block);