Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue #10306

Closed
wants to merge 1 commit into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Sep 16, 2022

In testKnownLimit(), we directly use the (pre-incremented) iv phi IV_PHI_i (232 Phi) in the loop exit check of the while loop:

Screenshot from 2022-09-16 10-33-58

Such pre-incremented iv phi uses after the loop are detected in PhaseIdealLoop::reorg_offsets() and replaced in order to reduce register pressure. We insert an additional Opaque2 node to prevent any optimizations to undo the effect of PhaseIdealLoop::reorg_offsets():

//     iv Phi            iv Phi
//       |                 |
//       |                AddI (+stride)
//       |                 |
//       |              Opaque2  # Blocks IGVN from folding these nodes until loop opts are over.
//       |     ====>       |
//       |                AddI (-stride)
//       |                 |
//       |               CastII  # Preserve type of iv Phi
//       |                 |
//   Outside Use       Outside Use

In the test case, this is done before CCP and looks like this:

Screenshot from 2022-09-16 10-33-35

At that point, we do not know yet that the while loop is only gonna be executed once (i.e. 422 CountedLoopEnd is always false). This only becomes known after CCP where the type of 232 Phi improves. But since we have an Opaque2 node, this update is not propagated until the Opaque2 nodes are removed in macro expansion:

} else if (n->is_Opaque1() || n->Opcode() == Op_Opaque2) {
_igvn.replace_node(n, n->in(1));
success = true;

During macro expansion, we also adjust the strip mined loop: We move the 421 Bool of the inner loop exit check 422 CountedLoopEnd to the outer strip mined loop exit check and adjust the inner loop exit check in such a way that C2 cannot figure out that the entire loop is only run once. In the next IGVN phase, the outer strip mined loop node is removed while the inner loop 429 CountedLoop is not.

Later in verify_strip_mined(), we cannot find the outer strip mined loop of 429 CountedLoop anymore and we fail with the assertion.

The first thought to fix this problem is to add Opaque2::Value() to let type information flow. But this does not fix the problem completely if the type of the iv phi has no known upper limit. There we have the problem that in general type(phi) != type(phi + x - x) because phi + x could overflow and we end up with type int (which happens in testUnknownLimit()).

I therefore suggest to remove Opaque2 nodes earlier before macro expansion to fix this bug. A good place seems to be right after loop opts are over. We can remove them at the same time as Opaque1 nodes by adding a similar Identity() method. This lets the loop nodes to be folded away before trying to adjust the outer strip mined loop limit.

Are Opaque2 nodes really useful?

When working on this bug, I started to question the usage of Opaque2 nodes in general. We are still running IGVN after Opaque2 nodes are currently removed. This simply undoes the effects of PhaseIdealLoop::reorg_offsets() again and we end up using pre-incremented iv phis anyways. My theory was that we are either blocking some specific optimizations during loop opts which cannot be reverted later in IGVN or that we initially (when this Opaque2 optimization was added) did not run IGVN anymore once Opaque2 nodes are removed.

I could not think of any such non-revertable optimization that Opaque2 nodes could prevent. On top of that, PhaseIdealLoop::reorg_offsets() also does not mention anything alike. I therefore had a look at the history of Opaque2 nodes. Unfortunately, they were added before the initial load commit. I've dug deeper through some old closed repo and found that at the time the Opaque2 nodes were introduced around 20 years ago, we did not do any IGVN anymore after the removal of the Opaque2 nodes - and we generated code with these unoptimized iv phi + x - x patterns.

This suggests that today the Opaque2 nodes are indeed not really doing what they were originally supposed to do. I would therefore suggest to investigate their complete removal in a separate RFE and go with the suggested fix above which does not make the current situation of the questionable Opaque2 node usages any worse.

Thanks,
Christian


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-8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10306

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 16, 2022

👋 Welcome back chagedorn! 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 openjdk bot added the rfr Pull request is ready for review label Sep 16, 2022
@openjdk
Copy link

openjdk bot commented Sep 16, 2022

@chhagedorn 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.org label Sep 16, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 16, 2022

Webrevs

Copy link
Contributor

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

@rwestrel
Copy link
Contributor

This suggests that today the Opaque2 nodes are indeed not really doing what they were originally supposed to do. I would therefore suggest to investigate their complete removal in a separate RFE and go with the suggested fix above which does not make the current situation of the questionable Opaque2 node usages any worse.

Makes sense to me.

@openjdk
Copy link

openjdk bot commented Sep 16, 2022

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

8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue

Reviewed-by: roland, kvn

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

  • 4b297c1: 8293892: Add links to JVMS 19 and 20 from ClassFileFormatVersion enum constants
  • dfb9c06: 8293535: jdk/javadoc/doclet/testJavaFX/TestJavaFxMode.java fail with jfx
  • f42caef: 8293550: Optionally add get-task-allow entitlement to macos binaries
  • 5feca68: 8293840: RISC-V: Remove cbuf parameter from far_call/far_jump/trampoline_call
  • 39cd163: 8293578: Duplicate ldc generated by javac
  • 7765942: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property
  • 11e7d53: 8293819: sun/util/logging/PlatformLoggerTest.java failed with "RuntimeException: Retrieved backing PlatformLogger level null is not the expected CONFIG"
  • 141d5f5: 8293767: AWT test TestSinhalaChar.java has old SCCS markings
  • 3beca2d: 8291600: [vectorapi] vector cast op check is not always needed for vector mask cast
  • 9a40b76: 8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options
  • ... and 52 more: https://git.openjdk.org/jdk/compare/37df5f56259429482cfdbe38e8b6256f1efaf9e8...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 Sep 16, 2022
@chhagedorn
Copy link
Member Author

Thanks Roland for your review and your feedback to further investigate the removal of Opaque2 nodes.

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.

Good.

@chhagedorn
Copy link
Member Author

Thanks Vladimir for your review!

/integrate

@openjdk
Copy link

openjdk bot commented Sep 19, 2022

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

  • a93cf92: 8293920: G1: Add index based heap region iteration
  • 36c9034: 8293808: mscapi destroyKeyContainer enhance KeyStoreException: Access is denied exception
  • cbd0688: 8293851: hs_err should print more stack in hex dump
  • 04d7b7d: 8293503: gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64 failed assertGreaterThanOrEqual: expected MMM >= NNN
  • d77c464: 8293891: gc/g1/mixedgc/TestOldGenCollectionUsage.java (still) assumes that GCs take 1ms minimum
  • d7c1a76: 8293861: G1: Disable preventive GCs by default
  • 43f7f47: 8293499: Provide jmod --compress option
  • 26e08cf: 8293844: C2: Verify Location::{oop,normal} types in PhaseOutput::FillLocArray
  • 357a2cc: 8293937: x86: Drop LP64 conditions from clearly x86_32 code
  • b1ed40a: 8293466: libjsig should ignore non-modifying sigaction calls
  • ... and 68 more: https://git.openjdk.org/jdk/compare/37df5f56259429482cfdbe38e8b6256f1efaf9e8...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 19, 2022
@openjdk openjdk bot closed this Sep 19, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 19, 2022
@openjdk
Copy link

openjdk bot commented Sep 19, 2022

@chhagedorn Pushed as commit 471e2f1.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
3 participants