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

[Damon Fenacci]

On Thu, 7 Nov 2024 00:01:01 GMT, Vladimir Ivanov <vlivanov 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)
>
> The root cause of the bug is that type information obtained during inlining is not propagated until IGVN kicks in. Vector API is special here, because (1) it heavily relies on exact type information to perform intrinsification; and (2) vector intrinsics are processed during post-parse inlining. IMO the current fix (do cleanup when VectorBox is returned) is good enough as a stop-the-gap fix for Vector API issue (missed intrinsification opportunity). 
> 
> As an alternative fix, limited IGVN pass over `CastPP`/`CheckCastPP` users of result value may be enough to avoid full-blown cleanup.
> 
> I suspect some other intrinsics may be susceptible to a similar issue, but in such case it would be more like a corner case (few intrinsics fail in rare conditions). A proper fix would be to re-examine failed intrinsics call site during IGVN and repeat intrinsifcation attempt when their inputs improve (akin to what is done in `CallStaticJavaNode::Ideal()`/`CallDynamicJavaNode::Ideal()`).

@iwanowww, @TobiHartmann thanks so much for your reviews!

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

PR Comment: https://git.openjdk.org/jdk/pull/21682#issuecomment-2753424128