fix simplify-locals bug where we create a br_if value, which is dangerous if we are moving code out of the br_if's condition - the value executes before (#1213)

3 files changed, 142 insertions, 0 deletions

diff --git a/src/passes/SimplifyLocals.cpp b/src/passes/SimplifyLocals.cpp
index cc74220df..cf64517b7 100644
--- a/src/passes/SimplifyLocals.cpp
+++ b/src/passes/SimplifyLocals.cpp
@@ -47,6 +47,7 @@
 #include <pass.h>
 #include <ast/count.h>
 #include <ast/effects.h>
+#include <ast/find_all.h>
 #include <ast/manipulation.h>
 
 namespace wasm {
@@ -327,6 +328,51 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals>>
       }
     }
     if (!found) return;
+    // If one of our brs is a br_if, then we will give it a value. since
+    // the value executes before the condition, it is dangerous if we are
+    // moving code out of the condition,
+    //  (br_if
+    //   (block
+    //    ..use $x..
+    //    (set_local $x ..)
+    //   )
+    //  )
+    // =>
+    //  (br_if
+    //   (tee_local $x ..) ;; this now affects the use!
+    //   (block
+    //    ..use $x..
+    //   )
+    //  )
+    // so we must check for that.
+    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* brp = breaks[j].brp;
+      auto* br = (*brp)->cast<Break>();
+      auto* set = (*breakSetLocalPointer)->cast<SetLocal>();
+      if (br->condition) {
+        // TODO: optimize
+        FindAll<SetLocal> findAll(br->condition);
+        for (auto* otherSet : findAll.list) {
+          if (otherSet == set) {
+            // the set is indeed in the condition, so we can't just move it
+            // but maybe there are no effects? see if, ignoring the set
+            // itself, there is any risk
+            Nop nop;
+            *breakSetLocalPointer = &nop;
+            EffectAnalyzer condition(getPassOptions(), br->condition);
+            EffectAnalyzer value(getPassOptions(), set);
+            *breakSetLocalPointer = set;
+            if (condition.invalidates(value)) {
+              // indeed, we can't do this, stop
+              return;
+            }
+            break; // we found set in the list, can stop now
+          }
+        }
+      }
+    }
     // Great, this local is set in them all, we can optimize!
     if (block->list.size() == 0 || !block->list.back()->is<Nop>()) {
       // We can't do this here, since we can't push to the block -
diff --git a/test/passes/simplify-locals.txt b/test/passes/simplify-locals.txt
index ae1e1ddba..9e5891434 100644
--- a/test/passes/simplify-locals.txt
+++ b/test/passes/simplify-locals.txt
@@ -1024,4 +1024,53 @@
    (get_local $x)
   )
  )
+ (func $br-value-reordering (type $FUNCSIG$i) (result i32)
+  (local $temp i32)
+  (block $outside
+   (loop $loop
+    (br_if $outside
+     (block $block (result i32)
+      (br_if $loop
+       (get_local $temp)
+      )
+      (unreachable)
+      (set_local $temp
+       (i32.const -1)
+      )
+      (i32.const 0)
+     )
+    )
+   )
+   (set_local $temp
+    (i32.const -1)
+   )
+  )
+  (unreachable)
+ )
+ (func $br-value-reordering-safe (type $FUNCSIG$i) (result i32)
+  (local $temp i32)
+  (set_local $temp
+   (block $outside (result i32)
+    (loop $loop
+     (drop
+      (get_local $temp)
+     )
+     (drop
+      (br_if $outside
+       (tee_local $temp
+        (i32.const -1)
+       )
+       (block $block (result i32)
+        (nop)
+        (i32.const 0)
+       )
+      )
+     )
+    )
+    (nop)
+    (i32.const -1)
+   )
+  )
+  (unreachable)
+ )
 )
diff --git a/test/passes/simplify-locals.wast b/test/passes/simplify-locals.wast
index 10fcf1277..842e2b5af 100644
--- a/test/passes/simplify-locals.wast
+++ b/test/passes/simplify-locals.wast
@@ -940,4 +940,51 @@
     (drop (i32.atomic.load (i32.const 1028)))
     (drop (get_local $x))
   )
+  (func $br-value-reordering (result i32)
+   (local $temp i32)
+   (block $outside
+    (loop $loop ;; we should exit this loop, hit the unreachable outside
+     ;; loop logic
+     (br_if $outside ;; we should not create a block value that adds a value to a br, if the value&condition of the br cannot be reordered,
+                     ;; as the value comes first
+      (block (result i32)
+       (br_if $loop
+        (get_local $temp) ;; false, don't loop
+       )
+       (unreachable) ;; the end
+       (set_local $temp
+        (i32.const -1)
+       )
+       (i32.const 0)
+      )
+     )
+    )
+    (set_local $temp
+     (i32.const -1)
+    )
+   )
+   (unreachable)
+  )
+  (func $br-value-reordering-safe (result i32)
+   (local $temp i32)
+   (block $outside
+    (loop $loop ;; we should exit this loop, hit the unreachable outside
+     ;; loop logic
+     (drop (get_local $temp)) ;; different from above - add a use here
+     (br_if $outside ;; we should not create a block value that adds a value to a br, if the value&condition of the br cannot be reordered,
+                     ;; as the value comes first
+      (block (result i32)
+       (set_local $temp ;; the use *is* in the condition, but it's ok, no conflicts
+        (i32.const -1)
+       )
+       (i32.const 0)
+      )
+     )
+    )
+    (set_local $temp
+     (i32.const -1)
+    )
+   )
+   (unreachable)
+  )
 )