wasm2js: unreachability fixes (#2037)

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

Author: kripken

check.py

@@ -27,7 +27,7 @@
     binary_format_check, delete_from_orbit, fail, fail_with_error,
     fail_if_not_identical, fail_if_not_contained, has_vanilla_emcc,
     has_vanilla_llvm, minify_check, options, tests, requested, warnings,
-    has_shell_timeout, fail_if_not_identical_to_file
+    has_shell_timeout, fail_if_not_identical_to_file, with_pass_debug
 )
 
 # For shared.num_failures. Cannot import directly because modifications made in
@@ -42,20 +42,6 @@
   assert os.path.exists(options.interpreter), 'interpreter not found'
 
 
-# run a check with BINARYEN_PASS_DEBUG set, to do full validation
-def with_pass_debug(check):
-  old_pass_debug = os.environ.get('BINARYEN_PASS_DEBUG')
-  try:
-    os.environ['BINARYEN_PASS_DEBUG'] = '1'
-    check()
-  finally:
-    if old_pass_debug is not None:
-      os.environ['BINARYEN_PASS_DEBUG'] = old_pass_debug
-    else:
-      if 'BINARYEN_PASS_DEBUG' in os.environ:
-        del os.environ['BINARYEN_PASS_DEBUG']
-
-
 def run_help_tests():
   print '[ checking --help is useful... ]\n'
 

scripts/test/shared.py

@@ -435,3 +435,17 @@ def minify_check(wast, verify_final_result=True):
 
 def files_with_pattern(*path_pattern):
   return sorted(glob.glob(os.path.join(*path_pattern)))
+
+
+# run a check with BINARYEN_PASS_DEBUG set, to do full validation
+def with_pass_debug(check):
+  old_pass_debug = os.environ.get('BINARYEN_PASS_DEBUG')
+  try:
+    os.environ['BINARYEN_PASS_DEBUG'] = '1'
+    check()
+  finally:
+    if old_pass_debug is not None:
+      os.environ['BINARYEN_PASS_DEBUG'] = old_pass_debug
+    else:
+      if 'BINARYEN_PASS_DEBUG' in os.environ:
+        del os.environ['BINARYEN_PASS_DEBUG']

scripts/test/wasm2js.py

@@ -19,7 +19,7 @@
 from support import run_command, split_wast
 from shared import (
     WASM2JS, MOZJS, NODEJS, fail_if_not_identical, options, tests,
-    fail_if_not_identical_to_file
+    fail_if_not_identical_to_file, with_pass_debug
 )
 
 # tests with i64s, invokes, etc.
@@ -73,6 +73,8 @@ def test_wasm2js_output():
 
       cmd += ['--allow-asserts']
       out = run_command(cmd)
+      # also verify it passes pass-debug verifications
+      with_pass_debug(lambda: run_command(cmd))
 
       open('a.2asm.asserts.mjs', 'w').write(out)
 

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,13 +273,15 @@ 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;
     setOutParam(curr, fetchOutParam(curr->body));
   }
 
   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 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
 
   // 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);

test/passes/flatten_i64-to-i32-lowering.txt

@@ -206,7 +206,7 @@
   (local $1 i32)
   (local $1$hi i32)
   (local $i64toi32_i32$0 i32)
-  (block $label$1
+  (block
    (unreachable)
    (unreachable)
   )
@@ -239,3 +239,61 @@
   (nop)
  )
 )
+(module
+ (type $0 (func (param i32 i32)))
+ (type $1 (func))
+ (global $f (mut i32) (i32.const -1412567121))
+ (global $g (mut i32) (global.get $f))
+ (global $f$hi (mut i32) (i32.const 305419896))
+ (global $g$hi (mut i32) (global.get $f$hi))
+ (global $i64toi32_i32$HIGH_BITS (mut i32) (i32.const 0))
+ (export "exp" (func $1))
+ (func $call (; 0 ;) (type $0) (param $0 i32) (param $0$hi i32)
+  (nop)
+ )
+ (func $1 (; 1 ;) (type $1)
+  (local $0 i32)
+  (local $0$hi i32)
+  (local $i64toi32_i32$0 i32)
+  (block
+   (block
+    (local.set $0
+     (block (result i32)
+      (local.set $i64toi32_i32$0
+       (global.get $f$hi)
+      )
+      (global.get $f)
+     )
+    )
+    (local.set $0$hi
+     (local.get $i64toi32_i32$0)
+    )
+   )
+   (call $call
+    (block (result i32)
+     (local.set $i64toi32_i32$0
+      (local.get $0$hi)
+     )
+     (local.get $0)
+    )
+    (local.get $i64toi32_i32$0)
+   )
+   (nop)
+   (block
+    (global.set $f
+     (block (result i32)
+      (local.set $i64toi32_i32$0
+       (i32.const 287454020)
+      )
+      (i32.const 1432778632)
+     )
+    )
+    (global.set $f$hi
+     (local.get $i64toi32_i32$0)
+    )
+   )
+   (nop)
+  )
+  (nop)
+ )
+)

test/passes/flatten_i64-to-i32-lowering.wast

@@ -41,3 +41,13 @@
   )
  )
 )
+(module
+ (global $f (mut i64) (i64.const 0x12345678ABCDEFAF))
+ (global $g (mut i64) (global.get $f))
+ (func $call (param i64))
+ (func "exp"
+  (call $call (global.get $f))
+  (global.set $f (i64.const 0x1122334455667788))
+ )
+)
+

test/wasm2js/block.2asm.js

@@ -135,10 +135,8 @@ function asmFunc(global, env, buffer) {
   $0 = 0;
   $1_1 = $0;
   block : {
-   block47 : {
-    $2_1 = 1;
-    break block;
-   }
+   $2_1 = 1;
+   break block;
   }
   $0 = $1_1 + $2_1 | 0;
   $5_1 = $0;
@@ -151,10 +149,8 @@ function asmFunc(global, env, buffer) {
   $0 = $9_1 + $10_1 | 0;
   $13_1 = $0;
   block51 : {
-   block52 : {
-    $14_1 = 8;
-    break block51;
-   }
+   $14_1 = 8;
+   break block51;
   }
   $0 = $13_1 + $14_1 | 0;
   return $0 | 0;

test/wasm2js/br.2asm.js

@@ -449,15 +449,14 @@ function asmFunc(global, env, buffer) {
  }
 
  function $47() {
-  var i64toi32_i32$0 = 0, $0 = 0, $0$hi = 0;
+  var $0 = 0, $0$hi = 0, i64toi32_i32$1 = 0;
   block : {
-   i64toi32_i32$0 = 0;
    $0 = 45;
-   $0$hi = i64toi32_i32$0;
+   $0$hi = 0;
    break block;
   }
-  i64toi32_i32$0 = $0$hi;
-  i64toi32_i32$HIGH_BITS = i64toi32_i32$0;
+  i64toi32_i32$1 = $0$hi;
+  i64toi32_i32$HIGH_BITS = i64toi32_i32$1;
   return $0 | 0;
  }
 
@@ -519,10 +518,8 @@ function asmFunc(global, env, buffer) {
  function $54() {
   var $0 = 0;
   block : {
-   block0 : {
-    $0 = 8;
-    break block;
-   }
+   $0 = 8;
+   break block;
   }
   return 1 + $0 | 0 | 0;
  }

test/wasm2js/br_table.2asm.js

@@ -49973,7 +49973,7 @@ function asmFunc(global, env, buffer) {
  }
 
  function $54() {
-  var i64toi32_i32$0 = 0, $1_1 = 0, $1$hi = 0;
+  var i64toi32_i32$0 = 0, $1_1 = 0, $1$hi = 0, i64toi32_i32$1 = 0;
   block : {
    i64toi32_i32$0 = 0;
    $1_1 = 45;
@@ -49983,8 +49983,8 @@ function asmFunc(global, env, buffer) {
     break block;
    };
   }
-  i64toi32_i32$0 = $1$hi;
-  i64toi32_i32$HIGH_BITS = i64toi32_i32$0;
+  i64toi32_i32$1 = $1$hi;
+  i64toi32_i32$HIGH_BITS = i64toi32_i32$1;
   return $1_1 | 0;
  }
 

test/wasm2js/br_table_temp.2asm.js

@@ -49977,7 +49977,6 @@ function asmFunc(global, env, buffer) {
     break block;
    };
   }
-  i64toi32_i32$0 = $1$hi;
   return $1_1 | 0;
  }