CoalesceLocals: ReFinalize when we refine a type (#6502)

The code had a different workaround, namely to add a block with an explicit
type to avoid changing the type at all (i.e. the block declared the original
type of the thing we replaced, not the refined type). But by adding a block we
can end up with an invalid pop, as the fuzzer found, see the EH testcase here.

To fix this, use the usual workaround of just running ReFinalize. That is simpler
and also improves the code while we do it. It does add more work, but this is
likely a very rare situation anyhow.

Author: kripken

src/passes/CoalesceLocals.cpp

@@ -105,6 +105,10 @@ struct CoalesceLocals
   bool interferes(Index i, Index j) {
     return interferences.get(std::min(i, j), std::max(i, j));
   }
+
+private:
+  // In some cases we need to refinalize at the end.
+  bool refinalize = false;
 };
 
 void CoalesceLocals::doWalkFunction(Function* func) {
@@ -118,6 +122,10 @@ void CoalesceLocals::doWalkFunction(Function* func) {
   pickIndices(indices);
   // apply indices
   applyIndices(indices, func->body);
+
+  if (refinalize) {
+    ReFinalize().walkFunctionInModule(func, getModule());
+  }
 }
 
 // A copy on a backedge can be especially costly, forcing us to branch just to
@@ -563,21 +571,13 @@ void CoalesceLocals::applyIndices(std::vector<Index>& indices,
             drop->value = value;
             *action.origin = drop;
           } else {
-            // This is a tee, and so, as earlier in this function, we must be
-            // careful of subtyping. Above we simply avoided the problem by
-            // leaving it for other passes, but we do want to remove ineffective
-            // stores - nothing else does that as well as this pass. Instead,
-            // create a block to cast back to the original type, which avoids
-            // changing types here, and leave it to other passes to refine types
-            // and remove the block.
             auto originalType = (*action.origin)->type;
             if (originalType != set->value->type) {
-              (*action.origin) =
-                Builder(*getModule()).makeBlock({set->value}, originalType);
-            } else {
-              // No special handling, just use the value.
-              *action.origin = set->value;
+              // The value had a more refined type, which we must propagate at
+              // the end.
+              refinalize = true;
             }
+            *action.origin = set->value;
           }
           continue;
         }

test/lit/passes/coalesce-locals-eh-old.wast

@@ -4,15 +4,20 @@
 (module
   ;; CHECK:      (tag $e)
 
-  ;; CHECK:      (func $bar (type $1) (result i32)
+  ;; CHECK:      (tag $any (param (ref any)))
+
+  ;; CHECK:      (func $bar (type $2) (result i32)
   ;; CHECK-NEXT:  (i32.const 1984)
   ;; CHECK-NEXT: )
   (func $bar (result i32)
     (i32.const 1984)
   )
 
  (tag $e)
-  ;; CHECK:      (func $bug-cfg-traversal (type $2) (param $0 i32) (result i32)
+
+ (tag $any (param (ref any)))
+
+  ;; CHECK:      (func $bug-cfg-traversal (type $3) (param $0 i32) (result i32)
   ;; CHECK-NEXT:  (try $try
   ;; CHECK-NEXT:   (do
   ;; CHECK-NEXT:    (local.set $0
@@ -42,4 +47,35 @@
     )
     (local.get $x)
   )
+
+  ;; CHECK:      (func $0 (type $0)
+  ;; CHECK-NEXT:  (local $0 anyref)
+  ;; CHECK-NEXT:  (try $try
+  ;; CHECK-NEXT:   (do
+  ;; CHECK-NEXT:    (nop)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (catch $any
+  ;; CHECK-NEXT:    (drop
+  ;; CHECK-NEXT:     (pop (ref any))
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $0
+    (local $0 (ref null any))
+    (try
+      (do)
+      (catch $any
+        (drop
+          ;; There is a difference between the type of the value here and the
+          ;; type of the local, due to the local being nullable. We should not
+          ;; error on that as we replace the tee with a drop (as it has no
+          ;; gets).
+          (local.tee $0
+            (pop (ref any))
+          )
+        )
+      )
+    )
+  )
 )

test/lit/passes/coalesce-locals-gc.wast

@@ -181,19 +181,16 @@
  )
 
  ;; CHECK:      (func $remove-tee-refinalize (type $5) (param $0 (ref null $A)) (param $1 (ref null $B)) (result structref)
- ;; CHECK-NEXT:  (struct.get $A 0
- ;; CHECK-NEXT:   (block (result (ref null $A))
- ;; CHECK-NEXT:    (local.get $1)
- ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  (struct.get $B 0
+ ;; CHECK-NEXT:   (local.get $1)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $remove-tee-refinalize
   (param $a (ref null $A))
   (param $b (ref null $B))
   (result (ref null struct))
-  ;; The local.tee receives a $B and flows out an $A. We want to avoid changing
-  ;; types here, so we'll wrap it in a block, and leave further improvements
-  ;; for other passes.
+  ;; The local.tee receives a $B and flows out an $A. We will ReFinalize here as
+  ;; we remove the tee, making the struct.get operate on $B.
   (struct.get $A 0
    (local.tee $a
     (local.get $b)
@@ -202,10 +199,8 @@
  )
 
  ;; CHECK:      (func $remove-tee-refinalize-2 (type $5) (param $0 (ref null $A)) (param $1 (ref null $B)) (result structref)
- ;; CHECK-NEXT:  (struct.get $A 0
- ;; CHECK-NEXT:   (block (result (ref null $A))
- ;; CHECK-NEXT:    (local.get $1)
- ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  (struct.get $B 0
+ ;; CHECK-NEXT:   (local.get $1)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $remove-tee-refinalize-2
@@ -311,9 +306,9 @@
  ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT:  (global.set $nn-tuple-global
- ;; CHECK-NEXT:   (block (type $1) (result (ref any) i32)
+ ;; CHECK-NEXT:   (block (type $0) (result (ref any) i32)
  ;; CHECK-NEXT:    (local.set $1
- ;; CHECK-NEXT:     (if (type $1) (result (ref any) i32)
+ ;; CHECK-NEXT:     (if (type $0) (result (ref any) i32)
  ;; CHECK-NEXT:      (i32.const 0)
  ;; CHECK-NEXT:      (then
  ;; CHECK-NEXT:       (tuple.make 2