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

[Vladimir Ivanov]

On Fri, 28 Feb 2025 13:37:13 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> # Issue
>> 
>> The `compiler/vectorapi/VectorLogicalOpIdentityTest.java` has been failing because C2 compiling the test `testAndMaskSameValue1` expects to have 1 `AndV` nodes but it has none.
>> 
>> # Cause
>> 
>> The issue has to do with the criteria that trigger a cleanup when performing late inlining. In the failing test, when the compiler tries to inline a `jdk.internal.vm.vector.VectorSupport::binaryOp` call, it fails because its argument is of the wrong type, mainly because some cast nodes “hide” the more “precise” type. 
>> The graph that leads to the issue looks like this:
>> ![1BCE8148-1E44-4CA1-AF8F-EFC6210FA740](https://github.com/user-attachments/assets/62dd917f-2dac-42a9-90cf-73eedcd3cf8a)
>> The compiler tries to inline `jdk.internal.vm.vector.VectorSupport::load` and it succeeds:
>> ![752E81C9-A37D-4626-81A9-E4A839FADD3D](https://github.com/user-attachments/assets/e61057b2-3093-4992-ba5a-b80e4000c0ec)
>> The node `3027 VectorBox` has type `IntMaxVector`. `912 CastPP` and `934 CheckCastPP` have type `IntVector`instead.
>> The compiler then tries to inline one of the 2 `bynaryOp` calls but it fails because it needs an argument of type `IntMaxVector` and the argument it is given, which is node `934 CheckCastPP` , has type `IntVector`.
>> 
>> This would not happen if between the 2 inlining attempts a _cleanup_ was triggered. IGVN would run and the 2 nodes  `912 CastPP` and `934 CheckCastPP` would be folded away. `binaryOp` could then be inlined since the types would match.
>> 
>> # Solution
>> 
>> Instead of fixing this specific case we try a more generic approach: when late inlining we keep track of failed intrinsics and re-examine them during IGVN. If the `Ideal` method for their call node is called, we reschedule the intrinsic attempt for that call.
>> 
>> # Testing
>> 
>> Additional test runs with `-XX:-TieredCompilation` are added to `VectorLogicalOpIdentityTest.java` and `VectorGatherMaskFoldingTest.java` as regression tests and `-XX:+IncrementalInlineForceCleanup` is removed from `VectorGatherMaskFoldingTest.java` (previously added as workaround for this issue)
>> 
>> Tests: Tier 1-4 (windows-x64, linux-x64/aarch64, and macosx-x64/aarch64; release and debug mode)
>
> 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

Nice! Overall, looks good.

Some minor comments/suggestions.

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?

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

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

Marked as reviewed by vlivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21682#pullrequestreview-2661688405
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r1981755545
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r1981791576