OptimizeInstructions: Do not change unreachability in if/select changes (#3840)

For example,

(if (result i32)
  (local.get $x)
  (return
    (local.get $y)
  )
  (return
    (local.get $z)
  )
)

If we move the returns outside we'd become unreachable, but we should
not make such type changes in this pass (they are handled by DCE and
Vacuum).

(found by the fuzzer)

1 files changed, 29 insertions, 2 deletions

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index c5ffd34b3..310924327 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -2874,9 +2874,10 @@ private:
           // TODO: consider the case with more than one child.
           ChildIterator ifTrueChildren(curr->ifTrue);
           if (ifTrueChildren.children.size() == 1) {
-            ChildIterator ifFalseChildren(curr->ifFalse);
             // 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 type.
+            // curr, and so there must be an LUB for curr to have a proper new
+            // type after the transformation.
+            //
             // An example where that does not happen is this:
             //
             //  (if
@@ -2884,10 +2885,35 @@ private:
             //    (drop (i32.const 1))
             //    (drop (f64.const 2.0))
             //  )
+            ChildIterator ifFalseChildren(curr->ifFalse);
             auto* ifTrueChild = *ifTrueChildren.begin();
             auto* ifFalseChild = *ifFalseChildren.begin();
             bool validTypes =
               Type::hasLeastUpperBound(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
+            // that further, and we leave such work to DCE and Vacuum anyhow.
+            // This can happen in something like this for example, where the
+            // outer type changes from i32 to unreachable if we move the
+            // returns outside:
+            //
+            //  (if (result i32)
+            //    (local.get $x)
+            //    (return
+            //      (local.get $y)
+            //    )
+            //    (return
+            //      (local.get $z)
+            //    )
+            //  )
+            assert(curr->ifTrue->type == curr->ifFalse->type);
+            auto newOuterType = curr->ifTrue->type;
+            if ((newOuterType == Type::unreachable) !=
+                (curr->type == Type::unreachable)) {
+              validTypes = false;
+            }
+
             // If the expression we are about to move outside has side effects,
             // then we cannot do so in general with a select: we'd be reducing
             // the amount of the effects as well as moving them. For an if,
@@ -2898,6 +2924,7 @@ private:
                                                        getModule()->features,
                                                        curr->ifTrue)
                                    .hasSideEffects();
+
             if (validTypes && validEffects) {
               // Replace ifTrue with its child.
               curr->ifTrue = ifTrueChild;