fix bug with turning if into br_if without checking for reordering dangers

5 files changed, 183 insertions, 30 deletions

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 924cd7848..8b7611ec9 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -25,6 +25,16 @@
 
 namespace wasm {
 
+// to turn an if into a br-if, we must be able to reorder the
+// condition and possible value, and the possible value must
+// not have side effects (as they would run unconditionally)
+static bool canTurnIfIntoBrIf(Expression* ifCondition, Expression* brValue) {
+  if (!brValue) return true;
+  EffectAnalyzer value(brValue);
+  if (value.hasSideEffects()) return false;
+  return !EffectAnalyzer(ifCondition).invalidates(value);
+}
+
 struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs, Visitor<RemoveUnusedBrs>>> {
   bool isFunctionParallel() override { return true; }
 
@@ -137,15 +147,11 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs, Visitor<R
       Break* br = curr->ifTrue->dynCast<Break>();
       if (br && !br->condition) { // TODO: if there is a condition, join them
         // if the br has a value, then if => br_if means we always execute the value, and also the order is value,condition vs condition,value
-        if (br->value) {
-          EffectAnalyzer value(br->value);
-          if (value.hasSideEffects()) return;
-          EffectAnalyzer condition(curr->condition);
-          if (condition.invalidates(value)) return;
+        if (canTurnIfIntoBrIf(curr->condition, br->value)) {
+          br->condition = curr->condition;
+          replaceCurrent(br);
+          anotherCycle = true;
         }
-        br->condition = curr->condition;
-        replaceCurrent(br);
-        anotherCycle = true;
       }
     }
     // TODO: if-else can be turned into a br_if as well, if one of the sides is a dead end
@@ -398,7 +404,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs, Visitor<R
           auto* iff = list[i]->dynCast<If>();
           if (!iff || !iff->ifFalse || isConcreteWasmType(iff->type)) continue; // if it lacked an if-false, it would already be a br_if, as that's the easy case
           auto* ifTrueBreak = iff->ifTrue->dynCast<Break>();
-          if (ifTrueBreak && !ifTrueBreak->condition) {
+          if (ifTrueBreak && !ifTrueBreak->condition && canTurnIfIntoBrIf(iff->condition, ifTrueBreak->value)) {
             // we are an if-else where the ifTrue is a break without a condition, so we can do this
             list[i] = ifTrueBreak;
             ifTrueBreak->condition = iff->condition;
@@ -407,7 +413,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs, Visitor<R
           }
           // otherwise, perhaps we can flip the if
           auto* ifFalseBreak = iff->ifFalse->dynCast<Break>();
-          if (ifFalseBreak && !ifFalseBreak->condition) {
+          if (ifFalseBreak && !ifFalseBreak->condition && canTurnIfIntoBrIf(iff->condition, ifFalseBreak->value)) {
             list[i] = ifFalseBreak;
             ifFalseBreak->condition = Builder(*getModule()).makeUnary(EqZInt32, iff->condition);
             ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifTrue);
diff --git a/test/emcc_hello_world.fromasm b/test/emcc_hello_world.fromasm
index 4680aea58..9f0d1ee47 100644
--- a/test/emcc_hello_world.fromasm
+++ b/test/emcc_hello_world.fromasm
@@ -3252,9 +3252,8 @@
                               (get_local $7)
                             )
                           )
-                          (br_if $label$break$L46
-                            (get_local $5)
-                            (i32.ge_u
+                          (if
+                            (i32.lt_u
                               (tee_local $9
                                 (i32.add
                                   (i32.load8_s
@@ -3270,15 +3269,18 @@
                               )
                               (i32.const 10)
                             )
-                          )
-                          (block
-                            (set_local $10
-                              (get_local $7)
+                            (block
+                              (set_local $10
+                                (get_local $7)
+                              )
+                              (set_local $7
+                                (get_local $9)
+                              )
+                              (br $while-in$18)
                             )
-                            (set_local $7
-                              (get_local $9)
+                            (br $label$break$L46
+                              (get_local $5)
                             )
-                            (br $while-in$18)
                           )
                         )
                       )
diff --git a/test/emcc_hello_world.fromasm.imprecise b/test/emcc_hello_world.fromasm.imprecise
index 66c49ce29..88532cef3 100644
--- a/test/emcc_hello_world.fromasm.imprecise
+++ b/test/emcc_hello_world.fromasm.imprecise
@@ -3245,9 +3245,8 @@
                               (get_local $7)
                             )
                           )
-                          (br_if $label$break$L46
-                            (get_local $5)
-                            (i32.ge_u
+                          (if
+                            (i32.lt_u
                               (tee_local $9
                                 (i32.add
                                   (i32.load8_s
@@ -3263,15 +3262,18 @@
                               )
                               (i32.const 10)
                             )
-                          )
-                          (block
-                            (set_local $10
-                              (get_local $7)
+                            (block
+                              (set_local $10
+                                (get_local $7)
+                              )
+                              (set_local $7
+                                (get_local $9)
+                              )
+                              (br $while-in$18)
                             )
-                            (set_local $7
-                              (get_local $9)
+                            (br $label$break$L46
+                              (get_local $5)
                             )
-                            (br $while-in$18)
                           )
                         )
                       )
diff --git a/test/passes/remove-unused-brs.txt b/test/passes/remove-unused-brs.txt
index 9eb413294..94b214588 100644
--- a/test/passes/remove-unused-brs.txt
+++ b/test/passes/remove-unused-brs.txt
@@ -3,6 +3,7 @@
   (type $0 (func (param i32)))
   (type $1 (func))
   (type $2 (func (result i32)))
+  (type $3 (func (param i32 i32) (result i32)))
   (func $b0-yes (type $0) (param $i1 i32)
     (block $topmost
     )
@@ -760,4 +761,86 @@
       )
     )
   )
+  (func $if-to-br_if-conflict (type $3) (param $x i32) (param $y i32) (result i32)
+    (block $leave
+      (set_local $y
+        (block $out
+          (if
+            (get_local $x)
+            (br $out
+              (block $block1
+                (set_local $x
+                  (i32.const 0)
+                )
+                (i32.const 1)
+              )
+            )
+            (br_if $leave
+              (i32.const 1)
+            )
+          )
+          (unreachable)
+        )
+      )
+    )
+    (i32.add
+      (get_local $x)
+      (get_local $y)
+    )
+  )
+  (func $if-to-br_if-conflict2 (type $3) (param $x i32) (param $y i32) (result i32)
+    (block $leave
+      (set_local $y
+        (block $out
+          (if
+            (get_local $x)
+            (br_if $leave
+              (i32.const 1)
+            )
+            (br $out
+              (block $block2
+                (set_local $x
+                  (i32.const 0)
+                )
+                (i32.const 1)
+              )
+            )
+          )
+          (unreachable)
+        )
+      )
+    )
+    (i32.add
+      (get_local $x)
+      (get_local $y)
+    )
+  )
+  (func $if-to-br_if-value-sideeffect (type $3) (param $x i32) (param $y i32) (result i32)
+    (block $leave
+      (set_local $y
+        (block $out
+          (if
+            (get_local $x)
+            (br $out
+              (block $block1
+                (drop
+                  (call $if-to-br_if-value-sideeffect
+                    (i32.const 0)
+                    (i32.const 1)
+                  )
+                )
+                (nop)
+                (i32.const 1)
+              )
+            )
+          )
+          (unreachable)
+        )
+      )
+    )
+    (i32.add
+      (get_local $x)
+      (get_local $y)
+    )
+  )
 )
diff --git a/test/passes/remove-unused-brs.wast b/test/passes/remove-unused-brs.wast
index e777efa5b..ecbcb6caa 100644
--- a/test/passes/remove-unused-brs.wast
+++ b/test/passes/remove-unused-brs.wast
@@ -700,4 +700,64 @@
       (br $stack1)
     )
   )
+  (func $if-to-br_if-conflict (param $x i32) (param $y i32) (result i32)
+    (block $leave
+      (set_local $y
+        (block $out
+          (if
+            (get_local $x)
+            (br $out
+              (block
+                (set_local $x (i32.const 0))
+                (i32.const 1)
+              )
+            )
+            (br_if $leave (i32.const 1))
+          )
+          (unreachable)
+        )
+      )
+    )
+    (i32.add (get_local $x) (get_local $y))
+  )
+  (func $if-to-br_if-conflict2 (param $x i32) (param $y i32) (result i32)
+    (block $leave
+      (set_local $y
+        (block $out
+          (if
+            (get_local $x)
+            (br_if $leave (i32.const 1))
+            (br $out
+              (block
+                (set_local $x (i32.const 0))
+                (i32.const 1)
+              )
+            )
+          )
+          (unreachable)
+        )
+      )
+    )
+    (i32.add (get_local $x) (get_local $y))
+  )
+  (func $if-to-br_if-value-sideeffect (param $x i32) (param $y i32) (result i32)
+    (block $leave
+      (set_local $y
+        (block $out
+          (if
+            (get_local $x)
+            (br $out
+              (block
+                (drop (call $if-to-br_if-value-sideeffect (i32.const 0) (i32.const 1)))
+                (nop)
+                (i32.const 1)
+              )
+            )
+          )
+          (unreachable)
+        )
+      )
+    )
+    (i32.add (get_local $x) (get_local $y))
+  )
 )