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

[Dean Long]

On Wed, 6 Nov 2024 08:06:33 GMT, Tobias Hartmann <thartmann 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
>> 
>> In order to fix this an extra cleanup has to be performed when we encounter a situation like the one above, i.e. when late inlining creates a `VectorBox`.
>> 
>> 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)
>
> src/hotspot/share/opto/callGenerator.cpp line 734:
> 
>> 732:     }
>> 733:     C->set_inlining_progress(true);
>> 734:     C->set_do_cleanup(kit.stopped() || result->Opcode() == Op_VectorBox); // path is dead or vector box; needs cleanup
> 
> This only triggers if the return value of the incrementally inlined method is a `VectorBox`, right? Is that sufficient? Could the `VectorBox` be hidden by another node?

I'm failing to understand why this is only an issue with VectorBox.  It doesn't feel quite right to be checking for a specific node type here.  Maybe this should be something like needs_cleanup(result)?

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

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