From c68861fbdfeebe8ef8dada7673ad798811540780 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Mon, 16 Aug 2021 11:30:39 -0700
Subject: [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.
---
 src/passes/OptimizeInstructions.cpp | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

(limited to 'src')

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
-- 
cgit v1.2.3