Fix br_if fallthrough value (#5200)

The fallthrough there is trickier because the value is evaluated before the condition.
Unlike other fallthroughs, the value is not last, so we need to check if the condition
(which is after it) interferes with it.

2 files changed, 62 insertions, 1 deletions

diff --git a/src/ir/properties.h b/src/ir/properties.h
index a32a737ef..bd0487d8f 100644
--- a/src/ir/properties.h
+++ b/src/ir/properties.h
@@ -286,8 +286,22 @@ inline Expression* getImmediateFallthrough(
       }
     }
   } else if (auto* br = curr->dynCast<Break>()) {
+    // Note that we must check for the ability to reorder the condition and the
+    // value, as the value is first, which would be a problem here:
+    //
+    //  (br_if ..
+    //    (local.get $x)    ;; value
+    //    (tee_local $x ..) ;; condition
+    //  )
+    //
+    // We must not say that the fallthrough value is $x, since it is the
+    // *earlier* value of $x before the tee that is passed out. But, if we can
+    // reorder then that means that the value could have been last and so we do
+    // know the fallthrough in that case.
     if (br->condition && br->value &&
-        behavior == FallthroughBehavior::AllowTeeBrIf) {
+        behavior == FallthroughBehavior::AllowTeeBrIf &&
+        EffectAnalyzer::canReorder(
+          passOptions, module, br->condition, br->value)) {
       return br->value;
     }
   } else if (auto* tryy = curr->dynCast<Try>()) {
diff --git a/test/lit/passes/simplify-locals_rse_fallthrough.wast b/test/lit/passes/simplify-locals_rse_fallthrough.wast
new file mode 100644
index 000000000..a9161a853
--- /dev/null
+++ b/test/lit/passes/simplify-locals_rse_fallthrough.wast
@@ -0,0 +1,47 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
+;; RUN: wasm-opt %s --simplify-locals --rse -S -o - \
+;; RUN:   | filecheck %s
+
+(module
+  ;; CHECK:      (func $no (param $x i32) (result i32)
+  ;; CHECK-NEXT:  (i32.add
+  ;; CHECK-NEXT:   (block $block (result i32)
+  ;; CHECK-NEXT:    (local.tee $x
+  ;; CHECK-NEXT:     (br_if $block
+  ;; CHECK-NEXT:      (local.get $x)
+  ;; CHECK-NEXT:      (local.tee $x
+  ;; CHECK-NEXT:       (i32.const 42)
+  ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (local.get $x)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $no (param $x i32) (result i32)
+    (i32.add
+      (block $block (result i32)
+        ;; This local.tee is necessary. It might seem that the br_if flows out
+        ;; $x, so we don't need to write that same value to $x once more.
+        ;; SimplifyLocals and RedundantSetElimination try to find such sets and
+        ;; remove them. But this set is necessary, since a tee of $x executes
+        ;; after that local.get. That is, the br_if flows out the *old* value of
+        ;; $x. What happens in practice is that we branch (since the condition
+        ;; is 42), and flow out the old $x, which gets tee'd into $x once more.
+        ;; As a result, the block flows out the original value of $x, and the
+        ;; local.get reads that same value, so the final add returns 2 times the
+        ;; original value of $x. For all that to happen, we need those
+        ;; optimization passes to *not* remove the outer local.tee.
+        (local.tee $x
+          (br_if $block
+            (local.get $x)
+            (local.tee $x
+              (i32.const 42)
+            )
+          )
+        )
+      )
+      (local.get $x)
+    )
+  )
+)