Optimize ref.as_non_null removal effect computation (#5479)

Followup to #5474

Author: kripken

src/passes/OptimizeInstructions.cpp

@@ -1500,21 +1500,27 @@ struct OptimizeInstructions
             // We need to see if a child with side effects exists after |input|.
             // If there is such a child, it is a problem as mentioned above (it
             // is fine for such a child to appear *before* |input|, as then we
-            // wouldn't be reordering effects).
+            // wouldn't be reordering effects). Thus, all we need to do is
+            // accumulate the effects in children after |input|, as we want to
+            // move the trap across those.
             bool seenInput = false;
+            EffectAnalyzer crossedEffects(options, *getModule());
             for (auto* child : ChildIterator(parent)) {
               if (child == input) {
                 seenInput = true;
               } else if (seenInput) {
-                // TODO We could ignore trap effects here (since traps are ok to
-                //      reorder) and also local effects (since a change to a var
-                //      would not be noticeable, unlike say a global).
-                if (EffectAnalyzer(options, *getModule(), child)
-                      .hasSideEffects()) {
-                  return;
-                }
+                crossedEffects.walk(child);
               }
             }
+
+            // Check if the effects we cross interfere with the effects of the
+            // trap we want to move. (We use a shallow effect analyzer since we
+            // will only move the ref.as_non_null itself.)
+            ShallowEffectAnalyzer movingEffects(options, *getModule(), input);
+            if (crossedEffects.invalidates(movingEffects)) {
+              return;
+            }
+
             // If we got here, we've checked the siblings and found no problem.
             checkedSiblings = true;
           }

test/lit/passes/optimize-instructions-gc-tnh.wast

@@ -608,6 +608,7 @@
   )
 
   ;; TNH:      (func $null.cast-other.effects (type $ref?|$struct|_=>_none) (param $x (ref null $struct))
+  ;; TNH-NEXT:  (local $i i32)
   ;; TNH-NEXT:  (struct.set $struct 0
   ;; TNH-NEXT:   (local.get $x)
   ;; TNH-NEXT:   (call $import)
@@ -616,8 +617,15 @@
   ;; TNH-NEXT:   (local.get $x)
   ;; TNH-NEXT:   (i32.const 10)
   ;; TNH-NEXT:  )
+  ;; TNH-NEXT:  (struct.set $struct 0
+  ;; TNH-NEXT:   (local.get $x)
+  ;; TNH-NEXT:   (local.tee $i
+  ;; TNH-NEXT:    (i32.const 10)
+  ;; TNH-NEXT:   )
+  ;; TNH-NEXT:  )
   ;; TNH-NEXT: )
   ;; NO_TNH:      (func $null.cast-other.effects (type $ref?|$struct|_=>_none) (param $x (ref null $struct))
+  ;; NO_TNH-NEXT:  (local $i i32)
   ;; NO_TNH-NEXT:  (struct.set $struct 0
   ;; NO_TNH-NEXT:   (ref.as_non_null
   ;; NO_TNH-NEXT:    (local.get $x)
@@ -628,8 +636,15 @@
   ;; NO_TNH-NEXT:   (local.get $x)
   ;; NO_TNH-NEXT:   (i32.const 10)
   ;; NO_TNH-NEXT:  )
+  ;; NO_TNH-NEXT:  (struct.set $struct 0
+  ;; NO_TNH-NEXT:   (local.get $x)
+  ;; NO_TNH-NEXT:   (local.tee $i
+  ;; NO_TNH-NEXT:    (i32.const 10)
+  ;; NO_TNH-NEXT:   )
+  ;; NO_TNH-NEXT:  )
   ;; NO_TNH-NEXT: )
   (func $null.cast-other.effects (param $x (ref null $struct))
+    (local $i i32)
     (struct.set $struct 0
       ;; We cannot remove this ref.as_non_null, even though the struct.set will
       ;; trap if the ref is null, because that would move the trap from before
@@ -648,6 +663,17 @@
       )
       (i32.const 10)
     )
+    (struct.set $struct 0
+      ;; This one can be removed even without TNH, even though there are effects
+      ;; in it. A tee only has local effects, which do not interfere with a
+      ;; trap.
+      (ref.as_non_null
+        (local.get $x)
+      )
+      (local.tee $i
+        (i32.const 10)
+      )
+    )
   )
 
   ;; Helper functions.