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)

2 files changed, 79 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;
diff --git a/test/lit/passes/optimize-instructions.wast b/test/lit/passes/optimize-instructions.wast
index 058c030dd..5b5b44b57 100644
--- a/test/lit/passes/optimize-instructions.wast
+++ b/test/lit/passes/optimize-instructions.wast
@@ -12188,6 +12188,33 @@
       )
     )
   )
+  ;; CHECK:      (func $ternary-identical-arms-return-select (param $x i32) (param $y i32) (param $z i32) (result i32)
+  ;; CHECK-NEXT:  (block $block
+  ;; CHECK-NEXT:   (select
+  ;; CHECK-NEXT:    (return
+  ;; CHECK-NEXT:     (local.get $x)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (return
+  ;; CHECK-NEXT:     (local.get $y)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (local.get $z)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $ternary-identical-arms-return-select (param $x i32) (param $y i32) (param $z i32) (result i32)
+    (block $block
+      ;; we cannot optimize a select currently as the return has side effects
+      (select
+        (return
+          (local.get $x)
+        )
+        (return
+          (local.get $y)
+        )
+        (local.get $z)
+      )
+    )
+  )
   ;; CHECK:      (func $send-i32 (param $0 i32)
   ;; CHECK-NEXT:  (nop)
   ;; CHECK-NEXT: )
@@ -12212,4 +12239,27 @@
       )
     )
   )
+  ;; CHECK:      (func $if-dont-change-to-unreachable (param $x i32) (param $y i32) (param $z i32) (result i32)
+  ;; CHECK-NEXT:  (if (result i32)
+  ;; CHECK-NEXT:   (local.get $x)
+  ;; CHECK-NEXT:   (return
+  ;; CHECK-NEXT:    (local.get $y)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (return
+  ;; CHECK-NEXT:    (local.get $z)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $if-dont-change-to-unreachable (param $x i32) (param $y i32) (param $z i32) (result i32)
+    ;; if we move the returns outside, we'd become unreachable; avoid that.
+    (if (result i32)
+      (local.get $x)
+      (return
+        (local.get $y)
+      )
+      (return
+        (local.get $z)
+      )
+    )
+  )
 )