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

dafedafe · 2024-10-24T13:31:37Z

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:

The compiler tries to inline jdk.internal.vm.vector.VectorSupport::load and it succeeds:

The node 3027 VectorBox has type IntMaxVector. 912 CastPP and 934 CheckCastPP have type IntVectorinstead.
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)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8302459: Missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure (Bug - P4)

Reviewers

Vladimir Ivanov (@iwanowww - Reviewer) Review applies to 9406c6e2
Tobias Hartmann (@TobiHartmann - Reviewer)

Contributors

Vladimir Ivanov <vlivanov@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21682/head:pull/21682
$ git checkout pull/21682

Update a local copy of the PR:
$ git checkout pull/21682
$ git pull https://git.openjdk.org/jdk.git pull/21682/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21682

View PR using the GUI difftool:
$ git pr show -t 21682

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21682.diff

Using Webrev

Link to Webrev Comment

…ed with "IRViolationException: There were one or multiple IR rule failures"

bridgekeeper · 2024-10-24T13:32:07Z

👋 Welcome back dfenacci! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-24T13:32:47Z

@dafedafe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Co-authored-by: Vladimir Ivanov <vlivanov@openjdk.org>
Reviewed-by: thartmann, vlivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 454 new commits pushed to the master branch:

de58009: 8351468: C2: array fill optimization assigns wrong type to intrinsic call
a875733: 8352486: [ubsan] compilationMemoryStatistic.cpp:659:21: runtime error: index 64 out of bounds for type const struct unnamed struct
5591f8a: 8351515: C2 incorrectly removes double negation for double and float
56a4ffa: 8352597: [IR Framework] test bug: TestNotCompilable.java fails on product build
e23e0f8: 8352591: Missing UnlockDiagnosticVMOptions in VerifyGraphEdgesWithDeadCodeCheckFromSafepoints test
adfb120: 8351748: Add class init barrier to AOT-cached Method/Var Handles
ee1577b: 8352652: [BACKOUT] nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called
df9210e: 8347706: jvmciEnv.cpp has jvmci includes out of order
5dd0acb: 8352477: RISC-V: Print warnings when unsupported intrinsics are enabled
334a1ee: 8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called
... and 444 more: https://git.openjdk.org/jdk/compare/a637ccf2fead25ea6a06ad6bd65e92b8694ee11c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-24T13:33:43Z

@dafedafe The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

This reverts commit a78798c.

This reverts commit f29f7a8.

This reverts commit 4131271.

This reverts commit 7ec8069.

This reverts commit 80901d4.

This reverts commit 3ba3823.

…ava failed with "IRViolationException: There were one or multiple IR rule failures"" This reverts commit 2f10a92.

…ctorLogicalOpIdentityTest.java IR failure

openjdk · 2024-11-04T09:29:34Z

@dafedafe this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8302459-new
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

mlbridge · 2024-11-04T15:08:47Z

Webrevs

dean-long · 2024-11-04T21:03:37Z

Would it be better to trigger cleanup based on the presence of nodes like CastPP/CheckCastPP instead?

iwanowww

Nice! Overall, looks good.

Some minor comments/suggestions.

iwanowww · 2025-03-05T16:22:30Z

src/hotspot/share/opto/callnode.cpp

+        phase->C->inline_printer()->record(cg->method(), cg->call_node()->jvms(), InliningResult::FAILURE,
+          "static call node changed: trying again");
      }
+      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?

It surely does. I added it.

iwanowww · 2025-03-05T16:42:57Z

src/hotspot/share/opto/compile.cpp

        print_method(PHASE_INCREMENTAL_INLINE_STEP, 3, cg->call_node());
        break; // process one call site at a time
+      } else {
+        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?)

Thanks a lot for having a look @iwanowww!

I took me a while to recollect it too (and I remember having a hard time figuring out if that could be an issue back then 😉). Anyway the concern, as you said, was that there might be an infinite loop between IGVN and incremental inlining (presumably because during incremental inlining the call node could potentially slip back into the working list, right?).
If that is the root of the problem, the issue would only exist in the false -> true case. In the (potential) true -> false case the call node wouldn't be scheduled for IGVN in the next round, so there wouldn't be any loop. Maybe we could even transform the statement into something like:

if (C->igvn_worklist()->member(cg->call_node()) && is_scheduled_for_igvn_before) { cg->call_node()->set_generator(cg); }

What do you think?

So, since current logic for generic (non-MH case) case conservatively assumes that any change in inputs may benefit inlining (and unconditionally schedules such call nodes for another inlining attempt during IGVN), we want to avoid the situation when call node gets scheduled for IGVN during failed inlining attempt.

I'd shape it as follows:

if (!is_scheduled_for_igvn_before && is_scheduled_for_igvn_after) { // avoid potential infinite loop assert(false, "scheduled for IGVN during inlining attempt"); } else { assert(is_scheduled_for_igvn_before == is_scheduled_for_igvn_after, "interesting"); // removed from IGVN list during inlining pass? cg->call_node()->set_generator(cg); // wait for another opportunity }

Fair enough. I reshaped it following your suggestion. Thanks!

iwanowww · 2025-03-07T19:23:13Z

src/hotspot/share/opto/callnode.cpp

 }
 #endif

+void CallJavaNode::prepend_and_reset_generator(PhaseGVN* phase, CallGenerator* cg) {


prepend_and_reset_generator sounds way too verbose. Maybe register_for_late_inline instead?

Also, CallGenertaor* cg argument is redundant. And phase is used just to extract Compile*.

Either

void CallJavaNode::register_for_late_inline(Compile* C) { if (generator() != nullptr) { C->prepend_late_inline(generator()); set_generator(nullptr); } else { assert(false, "repeated attempt"); } }

Or even drop Compile* C and use Compile::current().

prepend_and_reset_generator sounds way too verbose. Maybe register_for_late_inline instead?

👍

Or even drop Compile* C and use Compile::current().

I got rid of all the arguments. Thanks Vladimir.

iwanowww

Looks good!

TobiHartmann

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

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

src/hotspot/share/opto/callnode.cpp

TobiHartmann · 2025-03-20T12:49:51Z

src/hotspot/share/opto/callnode.cpp

+      assert(cg->method()->is_method_handle_intrinsic() == false, "required");
+      if (phase->C->print_inlining()) {
+        phase->C->inline_printer()->record(cg->method(), cg->call_node()->jvms(), InliningResult::FAILURE,
+          "static call node changed: trying again");


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

It looks like this:

@ 192 jdk.internal.vm.vector.VectorSupport::binaryOp (38 bytes) failed to inline: failed to inline (intrinsic) failed to inline: static call node changed: trying again (intrinsic) late inline succeeded

It seems a bit redundant: the first failed to inline: failed to inline (intrinsic) doesn't seem to be needed.

Actually it is a bit verbose but I would probably leave it like this: the first (failed to inline: failed to inline (intrinsic)) is for the failure and the second (failed to inline: static call node changed: trying again (intrinsic)) is for the retry.

src/hotspot/share/opto/callnode.cpp

TobiHartmann · 2025-03-20T13:05:19Z

src/hotspot/share/opto/compile.cpp

+        } else {
+          assert(is_scheduled_for_igvn_before == is_scheduled_for_igvn_after, "call node removed from IGVN list during inlining pass");
+          cg->call_node()->set_generator(cg);
+        }


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.

The difference is whether a call can be scheduled for a repeated inlining attempt in the future.

cg->call_node()->set_generator(cg) reinitializes cg in CallNode and lets IGVN to submit it for incremental inlining during future passes.

The first check guards against a situation when the call node is already on IGVN list (so, it will be automatically rescheduled for inlining during the next IGVN pass causing an infinite loop in incremental inlining).

The second assert catches a suspicious situation when the call node disappears from IGVN worklist during failed inlining attempt. IMO it should not happens, hence the assert. But it is benign to allow repeated inlining in such case.

Okay, thanks for the background. If we keep both asserts, some comments explaining this would be helpful.

Thanks for the suggestion!
I've added a couple of comments to the asserts.
I've also changed the second assert into assert(!is_scheduled_for_igvn_before || is_scheduled_for_igvn_after, "call node removed from IGVN list during inlining pass");. The final result is the same but the expression matches what the message says 1-1.

Looks good to me, thanks!

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

dafedafe · 2025-03-26T07:06:41Z

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

dafedafe · 2025-03-26T07:06:50Z

/integrate

openjdk · 2025-03-26T07:07:48Z

Going to push as commit 2e4d7d1.
Since your change was applied there have been 498 commits pushed to the master branch:

1a8c8e0: 8352858: Make java.net.JarURLConnection fields final
a81250c: 8352673: RISC-V: Vector can't be turned on with -XX:+UseRVV
0935ba9: 8352481: Enforce the use of lld with clang
dbc620f: 8352299: GenShen: Young cycles that interrupt old cycles cannot be cancelled
f5a0db4: 8315447: Invalid Type Annotation attached to a method instead of a lambda
60544a1: 8164714: Constructor.newInstance creates instance of inner class with null outer class
c856b34: 8352587: C2 SuperWord: we must avoid Multiversioning for PeelMainPost loops
993eae4: 8346948: Update CLDR to Version 47.0
e98838f: 8352065: [PPC64] C2: Implement PopCountVL, CountLeadingZerosV and CountTrailingZerosV nodes
03105fc: 8351601: [JMH] test UnixSocketChannelReadWrite failed for 2 threads config
... and 488 more: https://git.openjdk.org/jdk/compare/a637ccf2fead25ea6a06ad6bd65e92b8694ee11c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-26T07:07:56Z

@dafedafe Pushed as commit 2e4d7d1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

dafedafe added 5 commits October 24, 2024 08:28

JDK-8302459: compiler/vectorapi/VectorLogicalOpIdentityTest.java fail…

2f10a92

…ed with "IRViolationException: There were one or multiple IR rule failures"

JDK-8302459: remove unused vector inline queue

3ba3823

JDK-8302459: remove unneeded changes

80901d4

JDK-8302459: remove unneeded function declaration

7ec8069

JDK-8302459: add explicit -TieredCompilation to tests

4131271

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 24, 2024

dafedafe added 2 commits October 24, 2024 14:27

JDK-8302459: add bug numbers to tests

f29f7a8

JDK-8302459: update copyright year

a78798c

dafedafe changed the title ~~8302459: compiler/vectorapi/VectorLogicalOpIdentityTest.java failed with "IRViolationException: There were one or multiple IR rule failures"~~ 8302459: missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure Oct 25, 2024

dafedafe changed the title ~~8302459: missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure~~ 8302459: Missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure Oct 25, 2024

dafedafe added 8 commits November 4, 2024 08:39

Revert "JDK-8302459: update copyright year"

bffdbbd

This reverts commit a78798c.

Revert "JDK-8302459: add bug numbers to tests"

7481cc1

This reverts commit f29f7a8.

Revert "JDK-8302459: add explicit -TieredCompilation to tests"

15cbe15

This reverts commit 4131271.

Revert "JDK-8302459: remove unneeded function declaration"

0b7dff8

This reverts commit 7ec8069.

Revert "JDK-8302459: remove unneeded changes"

f547d89

This reverts commit 80901d4.

Revert "JDK-8302459: remove unused vector inline queue"

49c8a51

This reverts commit 3ba3823.

Revert "JDK-8302459: compiler/vectorapi/VectorLogicalOpIdentityTest.j…

11c63b5

…ava failed with "IRViolationException: There were one or multiple IR rule failures"" This reverts commit 2f10a92.

JDK-8302459: Missing late inline cleanup causes compiler/vectorapi/Ve…

63b749c

…ctorLogicalOpIdentityTest.java IR failure

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 4, 2024

Merge branch 'master' into JDK-8302459-new

bd488a9

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 4, 2024

dafedafe marked this pull request as ready for review November 4, 2024 15:04

openjdk bot added the rfr Pull request is ready for review label Nov 4, 2024

dafedafe added 3 commits February 27, 2025 12:33

JDK-8302459: update assert string

ba3547d

JDK-8302459: unneeded changes

2521e85

JDK-8302459: unneeded changes

e71e72f

dafedafe marked this pull request as ready for review February 28, 2025 13:32

openjdk bot added the rfr Pull request is ready for review label Feb 28, 2025

iwanowww approved these changes Mar 5, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 5, 2025

JDK-8302459: create method to prepend and reset generator

d688fcf

openjdk bot removed the ready Pull request is ready to be integrated label Mar 7, 2025

iwanowww reviewed Mar 7, 2025

View reviewed changes

dafedafe added 2 commits March 10, 2025 09:22

JDK-8302459: reshape infinite loop check

887dd96

JDK-8302459: refactor helper method

9406c6e

iwanowww approved these changes Mar 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 10, 2025

TobiHartmann approved these changes Mar 20, 2025

View reviewed changes

Update src/hotspot/share/opto/callnode.cpp

bbaf985

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Mar 24, 2025

dafedafe and others added 2 commits March 24, 2025 08:57

Update src/hotspot/share/opto/callnode.cpp

cfa5252

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

JDK-8302459: add comments to asserts

f84cafb

TobiHartmann approved these changes Mar 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 24, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 26, 2025

openjdk bot closed this Mar 26, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 26, 2025

dafedafe mentioned this pull request Apr 3, 2025

8352963: [REDO] Missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure #24402

Closed

3 tasks

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

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

Uh oh!

Conversation

dafedafe commented Oct 24, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Cause

Solution

Testing

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

openjdk bot commented Nov 4, 2024

Uh oh!

mlbridge bot commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dean-long commented Nov 4, 2024

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dafedafe commented Mar 26, 2025

Uh oh!

dafedafe commented Mar 26, 2025

Uh oh!

openjdk bot commented Mar 26, 2025

Uh oh!

dafedafe commented Oct 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 24, 2024 •

edited

Loading

mlbridge bot commented Nov 4, 2024 •

edited

Loading