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

[Tobias Hartmann]

On Mon, 10 Mar 2025 09:49:26 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 incrementally with two additional commits since the last revision:
> 
>  - JDK-8302459: refactor helper method
>  - JDK-8302459: reshape infinite loop check

This was a tricky one to narrow down, good job Damon! :slightly_smiling_face: 

I added a few code style comments, looks good otherwise.

src/hotspot/share/opto/callnode.cpp line 1114:

> 1112:     } else {
> 1113:       assert(IncrementalInline, "required");
> 1114:       assert(cg->method()->is_method_handle_intrinsic() == false, "required");

Suggestion:

      assert(!cg->method()->is_method_handle_intrinsic(), "required");

src/hotspot/share/opto/callnode.cpp line 1117:

> 1115:       if (phase->C->print_inlining()) {
> 1116:         phase->C->inline_printer()->record(cg->method(), cg->call_node()->jvms(), InliningResult::FAILURE,
> 1117:           "static call node changed: trying again");

FTR, could you share how the PrintInlining output looks now when this code is triggered?

src/hotspot/share/opto/callnode.cpp line 1215:

> 1213:       bool call_does_dispatch;
> 1214:       ciMethod* callee = phase->C->optimize_virtual_call(caller, klass, holder, orig_callee, receiver_type, true /*is_virtual*/,
> 1215:                                                         call_does_dispatch, not_used3);  // out-parameters

Suggestion:

                                                         call_does_dispatch, not_used3);  // out-parameters

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

> 2048:           assert(is_scheduled_for_igvn_before == is_scheduled_for_igvn_after, "call node removed from IGVN list during inlining pass");
> 2049:           cg->call_node()->set_generator(cg);
> 2050:         }

I find this a bit hard to read. Wouldn't it be semantically equivalent to this?


if (is_scheduled_for_igvn_before == is_scheduled_for_igvn_after) {
  cg->call_node()->set_generator(cg);
} else {
  assert(false, "Some useful message");
}


We wouldn't have separate asserts for the two cases, but I think that's fine since one can easily figure it out from the boolean values.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21682#pullrequestreview-2702546897
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r2005543474
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r2005569420
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r2005570672
PR Review Comment: https://git.openjdk.org/jdk/pull/21682#discussion_r2005598996