RemoveUnusedBrs: Refinalize when removing a break (#7918)

We refinalized when changing a break's type, but not when removing
it entirely. But that removal can affect other types, so we need to.

Looks like a regression from #7639, which was very hard for the fuzzer
to find. The issue is that we end up with types not fully refinalized, but
on control flow, that the validator doesn't fully check. The concrete
issue was some weird sequence of passes that leads to an assertion
inside SimplifyLocals; I extracted a simpler testcase using debug
annotations.

Author: kripken

src/passes/RemoveUnusedBrs.cpp

@@ -2024,8 +2024,6 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             block->list.push_back(curr);
             block->finalize();
             BranchHints::clear(curr, getFunction());
-            // The type changed, so refinalize.
-            refinalize = true;
           } else {
             // the branch is never taken, allow control flow to fall through
             if (curr->value) {
@@ -2034,6 +2032,11 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             }
           }
           replaceCurrent(block);
+          // The type changes if we make the break unconditional (it becomes
+          // unreachable), but a change may also happen if we see the break is
+          // not taken, as then the target block may have no more breaks, and
+          // become unreachable. Either way, refinalize.
+          refinalize = true;
         }
       }
     };

test/lit/debug/remove-unused-brs_full.wat

@@ -0,0 +1,70 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
+
+;; RUN: env BINARYEN_PRINT_FULL=1 wasm-opt %s --remove-unused-brs -S -o - | filecheck %s
+
+(module
+ ;; CHECK:      (func $test
+ ;; CHECK-NEXT:  ;;@
+ ;; CHECK-NEXT:  (loop $loop
+ ;; CHECK-NEXT:   ;;@
+ ;; CHECK-NEXT:   (if
+ ;; CHECK-NEXT:    ;;@
+ ;; CHECK-NEXT:    (i32.const 0) (; i32 ;)
+ ;; CHECK-NEXT:    (then
+ ;; CHECK-NEXT:     ;;@
+ ;; CHECK-NEXT:     (block $block (; unreachable ;)
+ ;; CHECK-NEXT:      ;;@
+ ;; CHECK-NEXT:      (br_if $loop
+ ;; CHECK-NEXT:       ;;@
+ ;; CHECK-NEXT:       (block (result i32) (; i32 ;)
+ ;; CHECK-NEXT:        ;;@
+ ;; CHECK-NEXT:        (block (; none ;)
+ ;; CHECK-NEXT:        ) ;; end block
+ ;; CHECK-NEXT:        ;;@
+ ;; CHECK-NEXT:        (i32.const 0) (; i32 ;)
+ ;; CHECK-NEXT:       ) ;; end block (; i32 ;)
+ ;; CHECK-NEXT:      ) (; none ;)
+ ;; CHECK-NEXT:      ;;@
+ ;; CHECK-NEXT:      (unreachable) (; unreachable ;)
+ ;; CHECK-NEXT:     ) ;; end block block (; unreachable ;)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:    (else
+ ;; CHECK-NEXT:     ;;@
+ ;; CHECK-NEXT:     (unreachable) (; unreachable ;)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:   ) ;; end if (; unreachable ;)
+ ;; CHECK-NEXT:  ) ;; end loop loop (; unreachable ;)
+ ;; CHECK-NEXT: )
+ (func $test
+  (loop $loop
+   (if
+    (i32.const 0)
+    (then
+     (block $block
+      (if
+       (block (result i32)
+        ;; This can be removed, after which $block has no breaks and it will
+        ;; become unreachable, as verified by PRINT_FULL. (The rest of the test
+        ;; around it is needed to avoid remove-unused-brs optimizing it all
+        ;; away as trivial.)
+        (br_if $block
+         (i32.const 0)
+        )
+        (i32.const 0)
+       )
+       (then
+        (br $loop)
+       )
+       (else
+        (unreachable)
+       )
+      )
+     )
+    )
+    (else
+     (unreachable)
+    )
+   )
+  )
+ )
+)