Skip to content

Commit 0cffeb5

Browse files
kripkenkripken
authored
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.
1 parent 670b736 commit 0cffeb5

File tree

6 files changed

+101
-16
lines changed

6 files changed

+101
-16
lines changed

‎src/passes/Inlining.cpp‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -404,14 +404,39 @@ static Expression* doInlining(Module* module,
404404
updater.walk(contents);
405405
block->list.push_back(contents);
406406
block->type = retType;
407-
// If the function returned a value, we just set the block containing the
408-
// inlined code to have that type. or, if the function was void and
409-
// contained void, that is fine too. a bad case is a void function in which
410-
// we have unreachable code, so we would be replacing a void call with an
411-
// unreachable.
412-
if (contents->type == Type::unreachable && block->type == Type::none) {
413-
// Make the block reachable by adding a break to it
414-
block->list.push_back(builder.makeBreak(block->name));
407+
// The ReFinalize below will handle propagating unreachability if we need to
408+
// do so, that is, if the call was reachable but now the inlined content we
409+
// replaced it with was unreachable. The opposite case requires special
410+
// handling: ReFinalize works under the assumption that code can become
411+
// unreachable, but it does not go back from that state. But inlining can
412+
// cause that:
413+
//
414+
// (call $A ;; an unreachable call
415+
// (unreachable)
416+
// )
417+
// =>
418+
// (block $__inlined_A_body (result i32) ;; reachable code after inlining
419+
// (unreachable)
420+
// )
421+
//
422+
// That is, if the called function wraps the input parameter in a block with a
423+
// declared type, then the block is not unreachable. And then we might error
424+
// if the outside expects the code to be unreachable - perhaps it only
425+
// validates that way. To fix this, if the call was unreachable then we make
426+
// the inlined code unreachable as well. That also maximizes DCE
427+
// opportunities by propagating unreachability as much as possible.
428+
//
429+
// (Note that we don't need to do this for a return_call, which is always
430+
// unreachable anyhow.)
431+
if (call->type == Type::unreachable && !call->isReturn) {
432+
// Make the replacement code unreachable. Note that we can't just add an
433+
// unreachable at the end, as the block might have breaks to it (returns are
434+
// transformed into those).
435+
Expression* old = block;
436+
if (old->type.isConcrete()) {
437+
old = builder.makeDrop(old);
438+
}
439+
*action.callSite = builder.makeSequence(old, builder.makeUnreachable());
415440
}
416441
// Anything we inlined into may now have non-unique label names, fix it up.
417442
// Note that we must do this before refinalization, as otherwise duplicate

‎test/lit/passes/inlining-unreachable.wast‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
;; CHECK: (func $call-trap (type $none_=>_none)
1717
;; CHECK-NEXT: (block $__inlined_func$trap
1818
;; CHECK-NEXT: (unreachable)
19-
;; CHECK-NEXT: (br $__inlined_func$trap)
2019
;; CHECK-NEXT: )
2120
;; CHECK-NEXT: )
2221
(func $call-trap
@@ -59,7 +58,6 @@
5958
;; CHECK-NEXT: (nop)
6059
;; CHECK-NEXT: (unreachable)
6160
;; CHECK-NEXT: )
62-
;; CHECK-NEXT: (br $__inlined_func$contents-then-trap)
6361
;; CHECK-NEXT: )
6462
;; CHECK-NEXT: )
6563
(func $call-contents-then-trap

‎test/lit/passes/inlining_all-features.wast‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,13 +142,11 @@
142142
;; CHECK: (func $1 (type $none_=>_none)
143143
;; CHECK-NEXT: (block $__inlined_func$0
144144
;; CHECK-NEXT: (unreachable)
145-
;; CHECK-NEXT: (br $__inlined_func$0)
146145
;; CHECK-NEXT: )
147146
;; CHECK-NEXT: )
148147
;; NOMNL: (func $1 (type $none_=>_none)
149148
;; NOMNL-NEXT: (block $__inlined_func$0
150149
;; NOMNL-NEXT: (unreachable)
151-
;; NOMNL-NEXT: (br $__inlined_func$0)
152150
;; NOMNL-NEXT: )
153151
;; NOMNL-NEXT: )
154152
(func $1

‎test/lit/passes/inlining_enable-tail-call.wast‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,6 @@
561561
;; CHECK-NEXT: (br $__inlined_func$1)
562562
;; CHECK-NEXT: )
563563
;; CHECK-NEXT: )
564-
;; CHECK-NEXT: (br $__inlined_func$1)
565564
;; CHECK-NEXT: )
566565
;; CHECK-NEXT: )
567566
(func $0
@@ -629,7 +628,6 @@
629628
;; CHECK-NEXT: )
630629
;; CHECK-NEXT: (br $__inlined_func$1)
631630
;; CHECK-NEXT: )
632-
;; CHECK-NEXT: (br $__inlined_func$1)
633631
;; CHECK-NEXT: )
634632
;; CHECK-NEXT: )
635633
(func $0
@@ -694,7 +692,6 @@
694692
;; CHECK-NEXT: )
695693
;; CHECK-NEXT: )
696694
;; CHECK-NEXT: )
697-
;; CHECK-NEXT: (br $__inlined_func$13)
698695
;; CHECK-NEXT: )
699696
;; CHECK-NEXT: )
700697
;; CHECK-NEXT: )

‎test/lit/passes/inlining_optimize-level=3.wast‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,3 +495,71 @@
495495
(unreachable)
496496
)
497497
)
498+
499+
;; We inline multiple times here, and in the sequence of those inlinings we
500+
;; turn the code in $B unreachable (when we inline $D), and no later inlining
501+
;; (of $C or $A, or even $C's inlining in $A) should turn it into anything else
502+
;; than an unreachable - once it is unreachable, we should keep it that way.
503+
;; (That avoids possible validation problems, and maximizes DCE.) To keep it
504+
;; unreachable we'll add an unreachable instruction after the inlined code.
505+
(module
506+
;; CHECK: (type $f32_=>_none (func (param f32)))
507+
508+
;; CHECK: (type $none_=>_none (func))
509+
510+
;; CHECK: (func $A (param $0 f32)
511+
;; CHECK-NEXT: (local $1 f32)
512+
;; CHECK-NEXT: (drop
513+
;; CHECK-NEXT: (block (result f32)
514+
;; CHECK-NEXT: (block $__inlined_func$C (result f32)
515+
;; CHECK-NEXT: (local.set $1
516+
;; CHECK-NEXT: (local.get $0)
517+
;; CHECK-NEXT: )
518+
;; CHECK-NEXT: (local.get $1)
519+
;; CHECK-NEXT: )
520+
;; CHECK-NEXT: )
521+
;; CHECK-NEXT: )
522+
;; CHECK-NEXT: )
523+
(func $A (param $0 f32)
524+
(drop
525+
(call $C
526+
(local.get $0)
527+
)
528+
)
529+
)
530+
;; CHECK: (func $B
531+
;; CHECK-NEXT: (local $0 f32)
532+
;; CHECK-NEXT: (call $A
533+
;; CHECK-NEXT: (block
534+
;; CHECK-NEXT: (block
535+
;; CHECK-NEXT: (drop
536+
;; CHECK-NEXT: (block $__inlined_func$C (result f32)
537+
;; CHECK-NEXT: (local.tee $0
538+
;; CHECK-NEXT: (block
539+
;; CHECK-NEXT: (block $__inlined_func$D
540+
;; CHECK-NEXT: (unreachable)
541+
;; CHECK-NEXT: )
542+
;; CHECK-NEXT: )
543+
;; CHECK-NEXT: )
544+
;; CHECK-NEXT: (local.get $0)
545+
;; CHECK-NEXT: )
546+
;; CHECK-NEXT: )
547+
;; CHECK-NEXT: (unreachable)
548+
;; CHECK-NEXT: )
549+
;; CHECK-NEXT: )
550+
;; CHECK-NEXT: )
551+
;; CHECK-NEXT: )
552+
(func $B
553+
(call $A
554+
(call $C
555+
(call $D)
556+
)
557+
)
558+
)
559+
(func $C (param $0 f32) (result f32)
560+
(local.get $0)
561+
)
562+
(func $D (result f32)
563+
(unreachable)
564+
)
565+
)

‎test/lit/passes/inlining_splitting.wast‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,6 @@
231231
;; CHECK-NEXT: (br $l)
232232
;; CHECK-NEXT: )
233233
;; CHECK-NEXT: )
234-
;; CHECK-NEXT: (br $__inlined_func$nondefaultable-param)
235234
;; CHECK-NEXT: )
236235
;; CHECK-NEXT: )
237236
(func $call-nondefaultable-param

0 commit comments

Comments
 (0)