RFR: 8319764: C2 compilation asserts during incremental inlining because Phi input is out of bounds [v2]

[Christian Hagedorn]

On Tue, 14 Nov 2023 11:26:50 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In the test case:
>> 
>> `lateInlined1()` and `lateInlined2()` are inlined incrementally
>> because they are invoked wih a method handle which is known constant
>> only after igvn. For the failure to reproduce, `lateInlined2()` must
>> be inlined after `lateInlined1()` which depends on igvn processing
>> order (that's why the test needs `StressIGVN`). `lateInlined1()` has 2
>> virtual calls that are inlined with bimorphic inlining. At each call
>> site, exception state is merged from `A.m()` and `B.m()`, the 2
>> methods that are inlined. `c` is casted to non null in `A.m()`. At the
>> first call site, `c` is live and merging the exception states causes
>> the creation of a `Phi` for `c` that merges `c` and the cast to non
>> null of `c`. At the second call site, `c` is not live so it's ignored
>> when merging exception states. When the exception states for the 2
>> call sites are combined, the region that merges the states at the
>> first call site is extended to have 4 inputs but the `Phi` for `c` at
>> the first call site is left with 2 inputs. When `lateInlined2()` is
>> inlined, it records a replaced node for a cast of `c` to non null. On
>> completion of incremental inlining, that replaced nodes is "applied".
>> In the process, the dead `Phi` for `c` created at the previous
>> incremental inline is found and because it has less inputs that its
>> region, the crash occurs.
>> 
>> The fix I propose is to simply ignore phi nodes that don't have the
>> same number of inputs as their region because they can only be a dead
>> Phi.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   -XX:+UnlockDiagnosticVMOptions

That looks reasonable to me, too!

src/hotspot/share/opto/replacednodes.cpp line 155:

> 153:         if (n->is_Phi()) {
> 154:           Node* region = n->in(0);
> 155:           if (n->req() == region->req()) { // dead phi?

Maybe you can change this comment to "ignore dead phis".

test/hotspot/jtreg/compiler/inlining/TestLateInlineReplacedNodesExceptionPath.java line 26:

> 24: /*
> 25:  * @test
> 26:  * bug 8319764

Suggestion:

 * @bug 8319764

test/hotspot/jtreg/compiler/inlining/TestLateInlineReplacedNodesExceptionPath.java line 29:

> 27:  * @summary C2 compilation asserts during incremental inlining because Phi input is out of bounds
> 28:  * @run main/othervm -XX:-TieredCompilation -XX:-BackgroundCompilation -XX:CompileCommand=dontinline,TestLateInlineReplacedNodesExceptionPath::notInlined
> 29:  *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressIGVN -XX:StressSeed=1246687813 TestLateInlineReplacedNodesExceptionPath

Since `StressIGVN` is C2 specific, you should add a `@requires vm.compiler2.enabled`.

-------------

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16648#pullrequestreview-1732144685
PR Review Comment: https://git.openjdk.org/jdk/pull/16648#discussion_r1394326576
PR Review Comment: https://git.openjdk.org/jdk/pull/16648#discussion_r1394269797
PR Review Comment: https://git.openjdk.org/jdk/pull/16648#discussion_r1394270753