8257575: C2: "failed: only phis" assert failure in loop strip mining verification

[rwestrel]

The loop in the test is a counted loop with a load pinned on the
backedge. The loop is strip mined. When the outer loop is expanded, it
ends up with a load on the backedge too as it must mirror the inner
loop. The load is input to a Phi. After expansion, the load on the
backedge of the outer loop is found to be rendundant with a dominating
load. That causes the Phi to be eliminated. The load on the backedge
of the inner loop is not eliminated. As a result, verification code
finds the inner and outer loops to have a different number of Phis
which is unexpected.

The load on the backedge of the inner loop could be eliminated. It's
not because it's not enqueued to be processed by the IGVN after loop
strip mining expansion. But beyond this immediate problem, because
IGVN processes each load separately, I think it's possible that it
succeeds in eliminating one of the loads but not the other (the graph
could have changed too much in the meantime). Because of that concern
and because it's a corner case, I think the most robust fix is to
relax the assert.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8257575: C2: "failed: only phis" assert failure in loop strip mining verification

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Nils Eliasson (@neliasso - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1555/head:pull/1555
$ git checkout pull/1555

[bridgekeeper]

👋 Welcome back roland! 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]

@rwestrel 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.

[mlbridge]

Webrevs

• 00: Full (3f84ba9)

[TobiHartmann]

Relaxing the assert seems reasonable but shouldn't we also fix the problem that the load is not eliminated?

[rwestrel]

Relaxing the assert seems reasonable but shouldn't we also fix the problem that the load is not eliminated?

I tried that but that didn't prove easy to make robust. For instance, I added code that enqueues the dominated load in the inner loop for IGVN but by the time it is processed, the dominated load's inputs have not yet all been processed so the dominated load is not found as dominating (mismatching address input). Maybe we could revert the logic in LoadNode::Ideal() so it looks for a dominated load rather than a dominating load. I gave it a try but I saw some loads were still not eliminated so I gave up given it seems like such a corner case.

[TobiHartmann]

Okay, thanks for the background information.

[openjdk]

@rwestrel 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:

8257575: C2: "failed: only phis" assert failure in loop strip mining verification

Reviewed-by: thartmann, neliasso

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 96 new commits pushed to the master branch:

• 05dac03: 8257803: Add -Xbatch to compiler/blackhole tests
• 29a09c8: 8257668: SA JMap - skip non-java thread stack dump for heap dump
• e590618: 8252505: C1/C2 compiler support for blackholes
• 972bc3b: 8256167: Convert JDK use of Reference::get to Reference::refersTo
• 78be334: 8242332: Add SHA3 support to SunPKCS11 provider
• c4339c3: 8243614: Typo in ReentrantLock's Javadoc
• d3ac1bf: 8198390: Test MultiResolutionDrawImageWithTransformTest.java fails when -esa is passed
• 51d325e: 8257633: Missing -mmacosx-version-min=X flag when linking libjvm
• e27ea4d: 8257750: writeBuffer field of java.io.DataOutputStream should be final
• dd0b945: 8257531: Super word not applied to a loop of simple Buffer operations
• ... and 86 more: https://git.openjdk.java.net/jdk/compare/eaf4db6b8b883b1ab5731ac62f590e39a7b67210...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.

[rwestrel]

@neliasso @TobiHartmann thanks for the reviews

[rwestrel]

/integrate

[openjdk]

@rwestrel Since your change was applied there have been 98 commits pushed to the master branch:

• d05401d: 8256679: Update serialization javadoc once JOSS changes for records are complete
• 7620124: 8257230: assert(InitialHeapSize >= MinHeapSize) failed: Ergonomics decided on incompatible initial and minimum heap sizes
• 05dac03: 8257803: Add -Xbatch to compiler/blackhole tests
• 29a09c8: 8257668: SA JMap - skip non-java thread stack dump for heap dump
• e590618: 8252505: C1/C2 compiler support for blackholes
• 972bc3b: 8256167: Convert JDK use of Reference::get to Reference::refersTo
• 78be334: 8242332: Add SHA3 support to SunPKCS11 provider
• c4339c3: 8243614: Typo in ReentrantLock's Javadoc
• d3ac1bf: 8198390: Test MultiResolutionDrawImageWithTransformTest.java fails when -esa is passed
• 51d325e: 8257633: Missing -mmacosx-version-min=X flag when linking libjvm
• ... and 88 more: https://git.openjdk.java.net/jdk/compare/eaf4db6b8b883b1ab5731ac62f590e39a7b67210...master

Your commit was automatically rebased without conflicts.

Pushed as commit f5a582c.

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