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

[Vladimir Ivanov]

On Fri, 7 Mar 2025 14:45:46 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> 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?

So, since current logic for generic (non-MH case) case conservatively assumes that any change in inputs may benefit inlining (and unconditionally schedules such call nodes for another inlining attempt during IGVN), we want to avoid the situation when call node gets scheduled for IGVN during failed inlining attempt. 

I'd shape it as follows:

if (!is_scheduled_for_igvn_before && is_scheduled_for_igvn_after) { // avoid potential infinite loop
  assert(false, "scheduled for IGVN during inlining attempt"); 
} else {
  assert(is_scheduled_for_igvn_before == is_scheduled_for_igvn_after, "interesting"); // removed from IGVN list during inlining pass?
  cg->call_node()->set_generator(cg); // wait for another opportunity
}

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

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