Merge pull request #775 from WebAssembly/sl-fix

Fix simplify-locals when adding a value to a br_if

4 files changed, 110 insertions, 34 deletions

diff --git a/scripts/fuzz_passes_wast.py b/scripts/fuzz_passes_wast.py
index 5ec032de1..7691cbbf5 100644
--- a/scripts/fuzz_passes_wast.py
+++ b/scripts/fuzz_passes_wast.py
@@ -53,10 +53,11 @@ args = sys.argv[2:]
 
 def run():
   try:
-    print 'run', ['bin/wasm-opt', wast]
-    subprocess.check_call(['bin/wasm-opt', wast])
+    cmd = ['bin/wasm-opt', wast]
+    print 'run', cmd
+    subprocess.check_call(cmd, stderr=open('/dev/null'))
   except Exception, e:
-    print ">>> !!! ", e, " !!!"
+    return ">>> !!! ", e, " !!!"
   return 'ok'
 
 original_wast = None
@@ -105,6 +106,7 @@ try:
   tested = set()
 
   def pick_passes():
+    # return '--waka'.split(' ')
     ret = []
     while 1:
       str_ret = str(ret)
diff --git a/src/passes/SimplifyLocals.cpp b/src/passes/SimplifyLocals.cpp
index acc6860be..17206af89 100644
--- a/src/passes/SimplifyLocals.cpp
+++ b/src/passes/SimplifyLocals.cpp
@@ -93,7 +93,7 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals,
   // sinkables. For the final exit from a block (falling off)
   // exitter is null.
   struct BlockBreak {
-    Break* br;
+    Expression** brp;
     Sinkables sinkables;
   };
 
@@ -128,7 +128,7 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals,
         // value means the block already has a return value
         self->unoptimizableBlocks.insert(br->name);
       } else {
-        self->blockBreaks[br->name].push_back({ br, std::move(self->sinkables) });
+        self->blockBreaks[br->name].push_back({ currp, std::move(self->sinkables) });
       }
     } else if (curr->is<Block>()) {
       return; // handled in visitBlock
@@ -290,7 +290,7 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals,
     auto breaks = std::move(blockBreaks[block->name]);
     blockBreaks.erase(block->name);
     if (breaks.size() == 0) return; // block has no branches TODO we might optimize trivial stuff here too
-    assert(!breaks[0].br->value); // block does not already have a return value (if one break has one, they all do)
+    assert(!(*breaks[0].brp)->cast<Break>()->value); // block does not already have a return value (if one break has one, they all do)
     // look for a set_local that is present in them all
     bool found = false;
     Index sharedIndex = -1;
@@ -328,7 +328,8 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals,
     for (size_t j = 0; j < breaks.size(); j++) {
       // move break set_local's value to the break
       auto* breakSetLocalPointer = breaks[j].sinkables.at(sharedIndex).item;
-      auto* br = breaks[j].br;
+      auto* brp = breaks[j].brp;
+      auto* br = (*brp)->cast<Break>();
       assert(!br->value);
       // if the break is conditional, then we must set the value here - if the break is not taken, we must still have the new value in the local
       auto* set = (*breakSetLocalPointer)->cast<SetLocal>();
@@ -336,6 +337,9 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals,
         br->value = set;
         set->setTee(true);
         *breakSetLocalPointer = getModule()->allocator.alloc<Nop>();
+        // in addition, as this is a conditional br that now has a value, it now returns a value, so it must be dropped
+        br->finalize();
+        *brp = Builder(*getModule()).makeDrop(br);
       } else {
         br->value = set->value;
         ExpressionManipulator::nop(set);
diff --git a/test/passes/simplify-locals.txt b/test/passes/simplify-locals.txt
index 4fed54992..36424193b 100644
--- a/test/passes/simplify-locals.txt
+++ b/test/passes/simplify-locals.txt
@@ -578,21 +578,25 @@
     (set_local $x
       (block $out i32
         (nop)
-        (br_if $out
-          (tee_local $x
-            (block $waka i32
-              (nop)
-              (br_if $waka
-                (tee_local $x
-                  (i32.const 12)
+        (drop
+          (br_if $out
+            (tee_local $x
+              (block $waka i32
+                (nop)
+                (drop
+                  (br_if $waka
+                    (tee_local $x
+                      (i32.const 12)
+                    )
+                    (i32.const 1)
+                  )
                 )
-                (i32.const 1)
+                (nop)
+                (i32.const 34)
               )
-              (nop)
-              (i32.const 34)
             )
+            (i32.const 1)
           )
-          (i32.const 1)
         )
         (drop
           (get_local $x)
@@ -613,21 +617,23 @@
           )
           (nop)
         )
-        (br_if $out
-          (tee_local $x
-            (if i32
-              (i32.const 1)
-              (block $block3 i32
-                (nop)
-                (i32.const 14)
-              )
-              (block $block5 i32
-                (nop)
-                (i32.const 25)
+        (drop
+          (br_if $out
+            (tee_local $x
+              (if i32
+                (i32.const 1)
+                (block $block3 i32
+                  (nop)
+                  (i32.const 14)
+                )
+                (block $block5 i32
+                  (nop)
+                  (i32.const 25)
+                )
               )
             )
+            (i32.const 1)
           )
-          (i32.const 1)
         )
         (block $sink-out-of-me-i-have-but-one-exit
           (nop)
@@ -720,11 +726,13 @@
             (get_local $a)
           )
           (nop)
-          (br_if $while-out$0
-            (tee_local $a
-              (i32.const 4)
+          (drop
+            (br_if $while-out$0
+              (tee_local $a
+                (i32.const 4)
+              )
+              (get_local $e)
             )
-            (get_local $e)
           )
           (nop)
           (i32.add
@@ -764,4 +772,36 @@
       (i32.const 0)
     )
   )
+  (func $drop-br_if (type $9) (param $label i32) (param $$cond2 i32) (param $$$0151 i32) (result i32)
+    (nop)
+    (tee_local $label
+      (block $label$break$L4 i32
+        (if
+          (i32.eq
+            (get_local $label)
+            (i32.const 15)
+          )
+          (block $block
+            (nop)
+            (nop)
+            (drop
+              (br_if $label$break$L4
+                (tee_local $label
+                  (i32.const 0)
+                )
+                (i32.eqz
+                  (i32.eq
+                    (get_local $$$0151)
+                    (i32.const 0)
+                  )
+                )
+              )
+            )
+          )
+        )
+        (nop)
+        (i32.const 1)
+      )
+    )
+  )
 )
diff --git a/test/passes/simplify-locals.wast b/test/passes/simplify-locals.wast
index c24f8bdc9..06907a570 100644
--- a/test/passes/simplify-locals.wast
+++ b/test/passes/simplify-locals.wast
@@ -801,4 +801,34 @@
       (i32.const 0)
     )
   )
+  (func $drop-br_if (param $label i32) (param $$cond2 i32) (param $$$0151 i32) (result i32)
+    (block $label$break$L4
+      (if
+        (i32.eq
+          (get_local $label)
+          (i32.const 15)
+        )
+        (block $block
+          (set_local $label
+            (i32.const 0)
+          )
+          (set_local $$cond2
+            (i32.eq
+              (get_local $$$0151)
+              (i32.const 0)
+            )
+          )
+          (br_if $label$break$L4 ;; when we add a value to this, its type changes as it returns the value too, so must be dropped
+            (i32.eqz
+              (get_local $$cond2)
+            )
+          )
+        )
+      )
+      (set_local $label
+        (i32.const 1)
+      )
+    )
+    (get_local $label)
+  )
 )