RFR: 8302459: Missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure [v3]

[Damon Fenacci]

On Wed, 5 Mar 2025 16:22:30 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> Damon Fenacci has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits:
>> 
>>  - JDK-8302459: unneeded changes
>>  - JDK-8302459: unneeded changes
>>  - JDK-8302459: update assert string
>>  - JDK-8302459: fix copyright year
>>  - JDK-8302459: fix after merge
>>  - Merge branch 'master' into JDK-8302459-new
>>  - JDK-8302459: add logging
>>  - JDK-8302459: remove todos
>>  - JDK-8302459: add check to avoid infinite loop
>>  - Merge branch 'master' into JDK-8302459-new
>>  - ... and 30 more: https://git.openjdk.org/jdk/compare/a637ccf2...e71e72f5
>
> src/hotspot/share/opto/callnode.cpp line 1112:
> 
>> 1110:           "static call node changed: trying again");
>> 1111:       }
>> 1112:       phase->C->prepend_late_inline(cg);
> 
> There are 4 occurrences of `prepend_late_inline` followed by `set_generator(nullptr)`. Does it deserve a helper method?

It surely does. I added it.

> src/hotspot/share/opto/compile.cpp line 2044:
> 
>> 2042:         break; // process one call site at a time
>> 2043:       } else {
>> 2044:         if (C->igvn_worklist()->member(cg->call_node()) == is_scheduled_for_igvn_before) { // avoid potential infinite loop
> 
> Can you remind me, please, what exactly we are trying to catch here?
> I remember I expressed concerns about the call node being scheduled for IGVN during incremental inlining attempt causing infinite loop during incremental inlining. Does the same apply if the node disappears from IGVN work list during incremental inlining attempt?
> 
> (It took me some time to recollect what's going on here. Maybe introduce `is_scheduled_for_igvn_after` local and add a comment why both mismatches - `false -> true` and `true -> false` - are problematic?)

Thanks a lot for having a look @iwanowww!

I took me a while to recollect it too (and I remember having a hard time figuring out if that could be an issue back then 😉). Anyway the concern, as you said, was that there might be an infinite loop between IGVN and incremental inlining (presumably because during incremental inlining the call node could potentially slip back into the working list, right?).
If that is the root of the problem, the issue would only exist in the `false -> true` case. In the (potential) `true -> false` case the call node wouldn't be scheduled for IGVN in the next round, so there wouldn't be any loop. Maybe we could even transform the statement into something like:

if (C->igvn_worklist()->member(cg->call_node()) && is_scheduled_for_igvn_before) {
  cg->call_node()->set_generator(cg);
}

What do you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r1985179449
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r1985179601