Never flip a call from unreachable to reachable during inlining (#5493)

See example in the new comment. In general, we never want to go from
unreachable to reachable (refinalize doesn't even try), as it misses out on
DCE opportunities. Also it may have validation issues, which is how the
fuzzer found this.

Remove code in the same place that was redundant after the refinalize
was added in #5492. That simplifies some existing testcases slightly, by
removing an unneeded br we added, and now we add a new unreachable
at the end in other cases that need it.

6 files changed, 101 insertions, 16 deletions

diff --git a/src/passes/Inlining.cpp b/src/passes/Inlining.cpp
index 7df5c8f29..1732c1a2c 100644
--- a/src/passes/Inlining.cpp
+++ b/src/passes/Inlining.cpp
@@ -404,14 +404,39 @@ static Expression* doInlining(Module* module,
   updater.walk(contents);
   block->list.push_back(contents);
   block->type = retType;
-  // If the function returned a value, we just set the block containing the
-  // inlined code to have that type. or, if the function was void and
-  // contained void, that is fine too. a bad case is a void function in which
-  // we have unreachable code, so we would be replacing a void call with an
-  // unreachable.
-  if (contents->type == Type::unreachable && block->type == Type::none) {
-    // Make the block reachable by adding a break to it
-    block->list.push_back(builder.makeBreak(block->name));
+  // The ReFinalize below will handle propagating unreachability if we need to
+  // do so, that is, if the call was reachable but now the inlined content we
+  // replaced it with was unreachable. The opposite case requires special
+  // handling: ReFinalize works under the assumption that code can become
+  // unreachable, but it does not go back from that state. But inlining can
+  // cause that:
+  //
+  //  (call $A                               ;; an unreachable call
+  //    (unreachable)
+  //  )
+  // =>
+  //  (block $__inlined_A_body (result i32)  ;; reachable code after inlining
+  //    (unreachable)
+  //  )
+  //
+  // That is, if the called function wraps the input parameter in a block with a
+  // declared type, then the block is not unreachable. And then we might error
+  // if the outside expects the code to be unreachable - perhaps it only
+  // validates that way. To fix this, if the call was unreachable then we make
+  // the inlined code unreachable as well. That also maximizes DCE
+  // opportunities by propagating unreachability as much as possible.
+  //
+  // (Note that we don't need to do this for a return_call, which is always
+  // unreachable anyhow.)
+  if (call->type == Type::unreachable && !call->isReturn) {
+    // Make the replacement code unreachable. Note that we can't just add an
+    // unreachable at the end, as the block might have breaks to it (returns are
+    // transformed into those).
+    Expression* old = block;
+    if (old->type.isConcrete()) {
+      old = builder.makeDrop(old);
+    }
+    *action.callSite = builder.makeSequence(old, builder.makeUnreachable());
   }
   // Anything we inlined into may now have non-unique label names, fix it up.
   // Note that we must do this before refinalization, as otherwise duplicate
diff --git a/test/lit/passes/inlining-unreachable.wast b/test/lit/passes/inlining-unreachable.wast
index e8797d5b9..66ed2097c 100644
--- a/test/lit/passes/inlining-unreachable.wast
+++ b/test/lit/passes/inlining-unreachable.wast
@@ -16,7 +16,6 @@
   ;; CHECK:      (func $call-trap (type $none_=>_none)
   ;; CHECK-NEXT:  (block $__inlined_func$trap
   ;; CHECK-NEXT:   (unreachable)
-  ;; CHECK-NEXT:   (br $__inlined_func$trap)
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT: )
   (func $call-trap
@@ -59,7 +58,6 @@
   ;; CHECK-NEXT:    (nop)
   ;; CHECK-NEXT:    (unreachable)
   ;; CHECK-NEXT:   )
-  ;; CHECK-NEXT:   (br $__inlined_func$contents-then-trap)
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT: )
   (func $call-contents-then-trap
diff --git a/test/lit/passes/inlining_all-features.wast b/test/lit/passes/inlining_all-features.wast
index d26a2aa9f..eee8fbdc9 100644
--- a/test/lit/passes/inlining_all-features.wast
+++ b/test/lit/passes/inlining_all-features.wast
@@ -142,13 +142,11 @@
  ;; CHECK:      (func $1 (type $none_=>_none)
  ;; CHECK-NEXT:  (block $__inlined_func$0
  ;; CHECK-NEXT:   (unreachable)
- ;; CHECK-NEXT:   (br $__inlined_func$0)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  ;; NOMNL:      (func $1 (type $none_=>_none)
  ;; NOMNL-NEXT:  (block $__inlined_func$0
  ;; NOMNL-NEXT:   (unreachable)
- ;; NOMNL-NEXT:   (br $__inlined_func$0)
  ;; NOMNL-NEXT:  )
  ;; NOMNL-NEXT: )
  (func $1
diff --git a/test/lit/passes/inlining_enable-tail-call.wast b/test/lit/passes/inlining_enable-tail-call.wast
index ed642594c..34b230371 100644
--- a/test/lit/passes/inlining_enable-tail-call.wast
+++ b/test/lit/passes/inlining_enable-tail-call.wast
@@ -561,7 +561,6 @@
  ;; CHECK-NEXT:     (br $__inlined_func$1)
  ;; CHECK-NEXT:    )
  ;; CHECK-NEXT:   )
- ;; CHECK-NEXT:   (br $__inlined_func$1)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $0
@@ -629,7 +628,6 @@
  ;; CHECK-NEXT:    )
  ;; CHECK-NEXT:    (br $__inlined_func$1)
  ;; CHECK-NEXT:   )
- ;; CHECK-NEXT:   (br $__inlined_func$1)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $0
@@ -694,7 +692,6 @@
  ;; CHECK-NEXT:       )
  ;; CHECK-NEXT:      )
  ;; CHECK-NEXT:     )
- ;; CHECK-NEXT:     (br $__inlined_func$13)
  ;; CHECK-NEXT:    )
  ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:  )
diff --git a/test/lit/passes/inlining_optimize-level=3.wast b/test/lit/passes/inlining_optimize-level=3.wast
index e5a0bc7e1..9e20e27dd 100644
--- a/test/lit/passes/inlining_optimize-level=3.wast
+++ b/test/lit/passes/inlining_optimize-level=3.wast
@@ -495,3 +495,71 @@
   (unreachable)
  )
 )
+
+;; We inline multiple times here, and in the sequence of those inlinings we
+;; turn the code in $B unreachable (when we inline $D), and no later inlining
+;; (of $C or $A, or even $C's inlining in $A) should turn it into anything else
+;; than an unreachable - once it is unreachable, we should keep it that way.
+;; (That avoids possible validation problems, and maximizes DCE.) To keep it
+;; unreachable we'll add an unreachable instruction after the inlined code.
+(module
+ ;; CHECK:      (type $f32_=>_none (func (param f32)))
+
+ ;; CHECK:      (type $none_=>_none (func))
+
+ ;; CHECK:      (func $A (param $0 f32)
+ ;; CHECK-NEXT:  (local $1 f32)
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (block (result f32)
+ ;; CHECK-NEXT:    (block $__inlined_func$C (result f32)
+ ;; CHECK-NEXT:     (local.set $1
+ ;; CHECK-NEXT:      (local.get $0)
+ ;; CHECK-NEXT:     )
+ ;; CHECK-NEXT:     (local.get $1)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $A (param $0 f32)
+  (drop
+   (call $C
+    (local.get $0)
+   )
+  )
+ )
+ ;; CHECK:      (func $B
+ ;; CHECK-NEXT:  (local $0 f32)
+ ;; CHECK-NEXT:  (call $A
+ ;; CHECK-NEXT:   (block
+ ;; CHECK-NEXT:    (block
+ ;; CHECK-NEXT:     (drop
+ ;; CHECK-NEXT:      (block $__inlined_func$C (result f32)
+ ;; CHECK-NEXT:       (local.tee $0
+ ;; CHECK-NEXT:        (block
+ ;; CHECK-NEXT:         (block $__inlined_func$D
+ ;; CHECK-NEXT:          (unreachable)
+ ;; CHECK-NEXT:         )
+ ;; CHECK-NEXT:        )
+ ;; CHECK-NEXT:       )
+ ;; CHECK-NEXT:       (local.get $0)
+ ;; CHECK-NEXT:      )
+ ;; CHECK-NEXT:     )
+ ;; CHECK-NEXT:     (unreachable)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $B
+  (call $A
+   (call $C
+    (call $D)
+   )
+  )
+ )
+ (func $C (param $0 f32) (result f32)
+  (local.get $0)
+ )
+ (func $D (result f32)
+  (unreachable)
+ )
+)
diff --git a/test/lit/passes/inlining_splitting.wast b/test/lit/passes/inlining_splitting.wast
index 2f840a494..9764aa235 100644
--- a/test/lit/passes/inlining_splitting.wast
+++ b/test/lit/passes/inlining_splitting.wast
@@ -231,7 +231,6 @@
   ;; CHECK-NEXT:     (br $l)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
-  ;; CHECK-NEXT:   (br $__inlined_func$nondefaultable-param)
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT: )
   (func $call-nondefaultable-param