[Wasm GC] Fix OptimizeInstructions on folding of identical code with nominal typing (#4069)

(if (result i32)
 (local.get $x)
 (struct.get $B 1
  (ref.null $B)
 )
 (struct.get $C 1
  (ref.null $C)
 )
)

With structural typing it is safe to turn this into this:

(struct.get $A 1
 (if (result (ref $A))
  (local.get $x)
  (ref.null $B)
  (ref.null $C)
 )
)

Here $A is the LUB of the others. This works since $A must have
field 1 in it. But with nominal types it is possible that the LUB in fact
does not have that field, and we would not validate.

This actually seems like a more general issue that might happen with
other things, even though atm perhaps it can't. For simplicity, avoid this
pattern in both nominal and structural typing, to avoid making a
difference between them.

2 files changed, 80 insertions, 4 deletions

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index e6854cf24..18fbf6d83 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -3127,21 +3127,44 @@ private:
           ChildIterator ifTrueChildren(curr->ifTrue);
           if (ifTrueChildren.children.size() == 1) {
             // ifTrue and ifFalse's children will become the direct children of
-            // curr, and so there must be an LUB for curr to have a proper new
+            // curr, and so they must be compatible to allow for a proper new
             // type after the transformation.
             //
-            // An example where that does not happen is this:
+            // At minimum an LUB is required, as shown here:
             //
             //  (if
             //    (condition)
             //    (drop (i32.const 1))
             //    (drop (f64.const 2.0))
             //  )
+            //
+            // However, that may not be enough, as with nominal types we can
+            // have things like this:
+            //
+            //  (if
+            //    (condition)
+            //    (struct.get $A 1 (..))
+            //    (struct.get $B 1 (..))
+            //  )
+            //
+            // It is possible that the LUB of $A and $B does not contain field
+            // "1". With structural types this specific problem is not possible,
+            // and it appears to be the case that with the GC MVP there is no
+            // instruction that poses a problem, but in principle it can happen
+            // there as well, if we add an instruction that returns the number
+            // of fields in a type, for example. For that reason, and to avoid
+            // a difference between structural and nominal typing here, disallow
+            // subtyping in both. (Note: In that example, the problem only
+            // happens because the type is not part of the struct.get - we infer
+            // it from the reference. That is why after hoisting the struct.get
+            // out, and computing a new type for the if that is now the child of
+            // the single struct.get, we get a struct.get of a supertype. So in
+            // principle we could fix this by modifying the IR as well, but the
+            // problem is more general, so avoid that.)
             ChildIterator ifFalseChildren(curr->ifFalse);
             auto* ifTrueChild = *ifTrueChildren.begin();
             auto* ifFalseChild = *ifFalseChildren.begin();
-            bool validTypes =
-              Type::hasLeastUpperBound(ifTrueChild->type, ifFalseChild->type);
+            bool validTypes = ifTrueChild->type == ifFalseChild->type;
 
             // In addition, after we move code outside of curr then we need to
             // not change unreachability - if we did, we'd need to propagate
diff --git a/test/lit/passes/optimize-instructions-gc.wast b/test/lit/passes/optimize-instructions-gc.wast
index 30f6a6c2d..27c966f52 100644
--- a/test/lit/passes/optimize-instructions-gc.wast
+++ b/test/lit/passes/optimize-instructions-gc.wast
@@ -18,10 +18,21 @@
   ;; NOMNL:      (type $empty (struct ))
   (type $empty (struct))
 
+  ;; CHECK:      (type $B (struct (field i32) (field i32) (field f32)))
+  ;; NOMNL:      (type $B (struct (field i32) (field i32) (field f32)) (extends $A))
+  (type $B (struct (field i32) (field i32) (field f32)) (extends $A))
+
+  ;; CHECK:      (type $C (struct (field i32) (field i32) (field f64)))
+  ;; NOMNL:      (type $C (struct (field i32) (field i32) (field f64)) (extends $A))
+  (type $C (struct (field i32) (field i32) (field f64)) (extends $A))
+
   ;; CHECK:      (type $array (array (mut i8)))
   ;; NOMNL:      (type $array (array (mut i8)))
   (type $array (array (mut i8)))
 
+  ;; NOMNL:      (type $A (struct (field i32)))
+  (type $A (struct (field i32)))
+
   ;; CHECK:      (import "env" "get-i32" (func $get-i32 (result i32)))
   ;; NOMNL:      (import "env" "get-i32" (func $get-i32 (result i32)))
   (import "env" "get-i32" (func $get-i32 (result i32)))
@@ -1612,4 +1623,46 @@
       )
     )
   )
+
+  ;; CHECK:      (func $hoist-LUB-danger (param $x i32) (result i32)
+  ;; CHECK-NEXT:  (if (result i32)
+  ;; CHECK-NEXT:   (local.get $x)
+  ;; CHECK-NEXT:   (struct.get $B 1
+  ;; CHECK-NEXT:    (ref.null $B)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (struct.get $C 1
+  ;; CHECK-NEXT:    (ref.null $C)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  ;; NOMNL:      (func $hoist-LUB-danger (param $x i32) (result i32)
+  ;; NOMNL-NEXT:  (if (result i32)
+  ;; NOMNL-NEXT:   (local.get $x)
+  ;; NOMNL-NEXT:   (struct.get $B 1
+  ;; NOMNL-NEXT:    (ref.null $B)
+  ;; NOMNL-NEXT:   )
+  ;; NOMNL-NEXT:   (struct.get $C 1
+  ;; NOMNL-NEXT:    (ref.null $C)
+  ;; NOMNL-NEXT:   )
+  ;; NOMNL-NEXT:  )
+  ;; NOMNL-NEXT: )
+  (func $hoist-LUB-danger (param $x i32) (result i32)
+    ;; In nominal typing, if we hoist the struct.get out of the if, then the if
+    ;; will have a new type, $A, but $A does not have field "1" which would be an
+    ;; error. We disallow subtyping for this reason.
+    ;;
+    ;; We also disallow subtyping in structural typing, even though atm there
+    ;; might not be a concrete risk there: future instructions might introduce
+    ;; such things, and it reduces the complexity of having differences with
+    ;; nominal typing.
+    (if (result i32)
+      (local.get $x)
+      (struct.get $B 1
+        (ref.null $B)
+      )
+      (struct.get $C 1
+        (ref.null $C)
+      )
+    )
+  )
 )