OptimizeInstructions: Fix static cast optimizations (#4311)

We found one cast that has another as its input, and forgot that
the child was possibly a fallthrough value. That is, there might be more
code that needs to be kept around.

Rather than fix the middle of the three cases there - the one with
HeapType::isSubType(childIntendedType, intendedType) - I
noticed it is not actually needed. That case checks if the child's
type is more specific than the parent's, and if so, then the parent
is not needed. But we already handle that earlier above in the
same function: regardless of what the child of a static cast is,
if the cast is static and the input is the proper type already, the
cast is unneeded (see lines 1565-1566).

1 files changed, 22 insertions, 8 deletions

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index a52126c5a..c6f1b0f3b 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -1608,20 +1608,34 @@ struct OptimizeInstructions
         auto childIntendedType = child->getIntendedType();
         if (HeapType::isSubType(intendedType, childIntendedType)) {
           // Skip the child.
-          curr->ref = child->ref;
-          return;
-        } else if (HeapType::isSubType(childIntendedType, intendedType)) {
-          // Skip the parent.
-          replaceCurrent(child);
-          return;
-        } else {
+          if (curr->ref == child) {
+            curr->ref = child->ref;
+            return;
+          } else {
+            // The child is not the direct child of the parent, but it is a
+            // fallthrough value, for example,
+            //
+            //  (ref.cast parent
+            //   (block
+            //    .. other code ..
+            //    (ref.cast child)))
+            //
+            // In this case it isn't obvious that we can remove the child, as
+            // doing so might require updating the types of the things in the
+            // middle - and in fact the sole purpose of the child may be to get
+            // a proper type for validation to work. Do nothing in this case,
+            // and hope that other opts will help here (for example,
+            // trapsNeverHappen will help if the code validates without the
+            // child).
+          }
+        } else if (!canBeCastTo(intendedType, childIntendedType)) {
           // The types are not compatible, so if the input is not null, this
           // will trap.
           if (!curr->type.isNullable()) {
             // Make sure to emit a block with the same type as us; leave
             // updating types for other passes.
             replaceCurrent(builder.makeBlock(
-              {builder.makeDrop(child->ref), builder.makeUnreachable()},
+              {builder.makeDrop(curr->ref), builder.makeUnreachable()},
               curr->type));
             return;
           }