EffectAnalyzer: Do not clear break targets before walk()/visit() (#5723)

We depend on repeated calls to walk/visit accumulating effects, so this
was a bug; if we want to clear stuff then we create a new EffectAnalyzer.

Removing that fixes the attached testcase. Also added a unit test.

3 files changed, 54 insertions, 7 deletions

diff --git a/src/ir/effects.h b/src/ir/effects.h
index b4987b12b..833da2540 100644
--- a/src/ir/effects.h
+++ b/src/ir/effects.h
@@ -53,14 +53,12 @@ public:
 
   // Walk an expression and all its children.
   void walk(Expression* ast) {
-    pre();
     InternalAnalyzer(*this).walk(ast);
     post();
   }
 
   // Visit an expression, without any children.
   void visit(Expression* ast) {
-    pre();
     InternalAnalyzer(*this).visit(ast);
     post();
   }
@@ -1024,11 +1022,6 @@ public:
   }
 
 private:
-  void pre() {
-    breakTargets.clear();
-    delegateTargets.clear();
-  }
-
   void post() {
     assert(tryDepth == 0);
 
diff --git a/test/example/cpp-unit.cpp b/test/example/cpp-unit.cpp
index d2339f030..e3e1269c3 100644
--- a/test/example/cpp-unit.cpp
+++ b/test/example/cpp-unit.cpp
@@ -637,6 +637,19 @@ void test_effects() {
     assert_equal(effects.readsMutableStruct, false);
     assert_equal(effects.writesStruct, false);
   }
+
+  {
+    EffectAnalyzer effects(options, module);
+
+    // If we break, then we transfer control flow.
+    effects.breakTargets.insert("block");
+    assert_equal(effects.transfersControlFlow(), true);
+
+    // Repeated walks accumulate effects, that is, old effects are not
+    // removed.
+    effects.walk(&nop);
+    assert_equal(effects.transfersControlFlow(), true);
+  }
 }
 
 void test_field() {
diff --git a/test/lit/passes/optimize-instructions-gc.wast b/test/lit/passes/optimize-instructions-gc.wast
index 4535a53b3..7872a10bc 100644
--- a/test/lit/passes/optimize-instructions-gc.wast
+++ b/test/lit/passes/optimize-instructions-gc.wast
@@ -2499,4 +2499,45 @@
   (func $struct_i64_helper (type $struct_i64) (param $0 (ref null struct)) (result i64)
     (unreachable)
   )
+
+  ;; CHECK:      (func $array-copy-non-null (type $ref?|$array|_=>_none) (param $x (ref null $array))
+  ;; CHECK-NEXT:  (block $block
+  ;; CHECK-NEXT:   (array.copy $array $array
+  ;; CHECK-NEXT:    (ref.as_non_null
+  ;; CHECK-NEXT:     (local.get $x)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (if (result i32)
+  ;; CHECK-NEXT:     (i32.const 1)
+  ;; CHECK-NEXT:     (br $block)
+  ;; CHECK-NEXT:     (i32.const 10)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (local.get $x)
+  ;; CHECK-NEXT:    (i32.const 42)
+  ;; CHECK-NEXT:    (i32.const 1337)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $array-copy-non-null (param $x (ref null $array))
+    (block $block
+      (array.copy $array $array
+        ;; This cast cannot be removed: while the array.copy will trap anyhow
+        ;; if $x is null, we might branch out in the if, so removing a trap
+        ;; here could be noticeable.
+        (ref.as_non_null
+          (local.get $x)
+        )
+        (if (result i32)
+          (i32.const 1)
+          (br $block)
+          (i32.const 10)
+        )
+        ;; There are no tricky effects after this, so this cast can be removed.
+        (ref.as_non_null
+          (local.get $x)
+        )
+        (i32.const 42)
+        (i32.const 1337)
+      )
+    )
+  )
 )