Skip to content
This repository has been archived by the owner on Sep 2, 2022. It is now read-only.
/ jdk17 Public archive

8271272: C2: assert(!had_error) failed: bad dominance #287

Closed
wants to merge 1 commit into from

Conversation

rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Jul 28, 2021

The test has a counted loop. The type of the iv Phi of the counted loop is set initially to: min+5..7. The loop body contains a ConvI2L of the iv Phi. That value is used as a value to be stored right out of the loop. That ConvI2L captures the type of the iv Phi: minint+5..7. Pre/main/post loops are created. The loop is unrolled once. The ConvI2L is cloned and pushed thru the Add of the iv. The increment of the loop is 2. The new ConvI2L now takes the iv Phi as input and has type: minint+5..5. CCP runs and updates the type of the iv Phi to: int:6..7. That type conflicts with the type of the second ConvI2L. That node is transformed to top which causes the Store to be transformed to top. The Store has a Phi as use. That Phi is transformed to the remaining non top input. That causes the assert failure.

This happens because the main loop body never executes. The entry test would constant fold if it didn't use an Opaque1.

The exit condition of the main loop will next constant fold, the backedge is removed. So the fix for JDK-8269752 should have prevented this issue because once the main loop backedge is removed, the Opaque1 of the entry test should also be removed. But I noticed working on this, that the fix for JDK-8269752 is not sufficient. In the case of JDK-8269752, both the main and post loops loose their backedge but, with the fix, only the Opaque1 for the post loop is removed. That's good enough for JDK-8269752 but not here. The reason the fix for JDK-8269752 doesn't trigger is that the main loop is strip mined. When the main loop looses its backedge, the logic that checks for an Opaque1 in the entry test of the main loop does trigger but because it sees a dying counted loop it doesn't properly step over the outer strip mined loop. The fix I propose changes the logic in CountedLoopNode::skip_strip_mined() so that it works with a dying counted loop node.

Tobias has started testing for this fix.


Progress

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

Issue

  • JDK-8271272: C2: assert(!had_error) failed: bad dominance

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/287/head:pull/287
$ git checkout pull/287

Update a local copy of the PR:
$ git checkout pull/287
$ git pull https://git.openjdk.java.net/jdk17 pull/287/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 287

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/287.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 28, 2021

👋 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.

@rwestrel rwestrel marked this pull request as draft July 28, 2021 14:36
@openjdk
Copy link

openjdk bot commented Jul 28, 2021

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.java.net label Jul 28, 2021
@rwestrel rwestrel marked this pull request as ready for review July 28, 2021 16:13
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 28, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 28, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice analysis. Looks good.

@openjdk
Copy link

openjdk bot commented Jul 28, 2021

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

8271272: C2: assert(!had_error) failed: bad dominance

Reviewed-by: kvn, thartmann, chagedorn

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

  • 20d2dc1: 8271403: mark hotspot runtime/memory tests which ignore external VM flags
  • e593e3d: 8271402: mark hotspot runtime/os tests which ignore external VM flags
  • 7bf72ce: 8271412: ProblemList javax/sound/midi/Sequencer/Looping.java
  • 6878b05: 8271251: JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6"
  • c8ae7e5: 8271174: runtime/ClassFile/UnsupportedClassFileVersion.java can be run in driver mode
  • 5fcf720: 8271352: Extend jcc erratum mitigation to additional processors
  • 6e1da64: 8270908: TestParallelRefProc fails on single core machines
  • f1e15c8: 8271350: runtime/Safepoint tests use OutputAnalyzer::shouldMatch instead of shouldContaint
  • fbe28e4: 8270866: NPE in DocTreePath.getTreePath()
  • f662127: 8270491: SEGV at read_string_field(oopDesc*, char const*, JavaThread*)+0x54
  • ... and 4 more: https://git.openjdk.java.net/jdk17/compare/049b2ad4dfbc13d21fd58467a0975097a17e8681...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 openjdk bot added the ready Pull request is ready to be integrated label Jul 28, 2021
Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Testing is still running, I'll get back once it completed.

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@TobiHartmann
Copy link
Member

Testing looks good.

@rwestrel
Copy link
Contributor Author

@vnkozlov @chhagedorn @TobiHartmann thanks for the reviews.

@rwestrel
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 30, 2021

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

  • 6180cf1: 8271512: ProblemList serviceability/sa/sadebugd/DebugdConnectTest.java due to 8270326
  • a1b5b81: 8271507: ProblemList SA tests that are failing with ZGC due to JDK-8248912
  • 286d313: 8271489: (doc) Clarify Filter Factory example
  • 20d2dc1: 8271403: mark hotspot runtime/memory tests which ignore external VM flags
  • e593e3d: 8271402: mark hotspot runtime/os tests which ignore external VM flags
  • 7bf72ce: 8271412: ProblemList javax/sound/midi/Sequencer/Looping.java
  • 6878b05: 8271251: JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6"
  • c8ae7e5: 8271174: runtime/ClassFile/UnsupportedClassFileVersion.java can be run in driver mode
  • 5fcf720: 8271352: Extend jcc erratum mitigation to additional processors
  • 6e1da64: 8270908: TestParallelRefProc fails on single core machines
  • ... and 7 more: https://git.openjdk.java.net/jdk17/compare/049b2ad4dfbc13d21fd58467a0975097a17e8681...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 30, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 30, 2021
@openjdk
Copy link

openjdk bot commented Jul 30, 2021

@rwestrel Pushed as commit e351de3.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.java.net integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants