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

[Quan Anh Mai]

On Wed, 4 Dec 2024 07:24:04 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.
>> 
>> 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)
>
> Damon Fenacci has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 28 commits:
> 
>  - Merge branch 'master' into JDK-8302459-new
>  - JDK-8302459: remove unneeded copyright year change
>  - JDK-8302459: add failed late inlines handling to dynamic calls
>  - JDK-8032459: fix indentation
>  - JDK-8302459: copyright year
>  - JDK-8302459: simplify late inline failure conditions
>  - JDK-8302459: revert unneeded copyright update
>  - JDK-8302459: remove unneeded spaces
>  - JDK-8302459: increment MH late inline counter when reinserting them
>  - Merge branch 'master' into JDK-8302459-new
>  - ... and 18 more: https://git.openjdk.org/jdk/compare/4fbf2720...7a4ffc11

src/hotspot/share/opto/compile.cpp line 2088:

> 2086:     igvn.reset_from_gvn(initial_gvn());
> 2087:     igvn.optimize();
> 2088:     // Reset failed generator in call node

This is actually run in the incremental inline loop. Typically, we inline calls until we have too many nodes, at which time we do the cleanup with IGVN and IdealLoop and try again with the remaining calls in the queue. I think your approach is similar to we just retrying inlining a second time for all inlining failures.

As a result, I suggest you moving this to the main loop of `Compile::inline_incrementally`, on each iteration, we start inlining all calls in the `failed_once` queue, then continue with the main queue as normal. For `Compile::inline_incrementally_one`, each node that fails inlining is pushed into the `failed_once` queue (note that failed inline after `LiveNodeCountInliningCutoff` being broken should not be pushed into the `failed_once` queue and left in the original queue.

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

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