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

8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint #2315

Closed
wants to merge 2 commits into from

Conversation

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Jan 29, 2021

Loop strip mining verification fails because a LoadNode with no safepoint use ends up in the OuterStripMinedLoop. The root cause is very similar to JDK-8249607: A LoadNode with two uses (a field store and a return, see test2) is cloned by PhaseIdealLoop::split_if_with_blocks_post() for each use, to allow it to flow out of the loop. Both clones end up in the OuterStripMinedLoop, the clone for the field store because the store has a safepoint use and the clone for the return because late_load_ctrl is too conservative by taking all initial uses into account.

Now the load without a safepoint use is always detected by loop strip mining verification but without verification (for example, in a product build), we still hit several different issues depending on the exact use of the load (compare test2/3/4). The main issue is that loads without a safepoint use are not correctly wired when creating pre/main/post loops. Christian described this well in his RFR for JDK-8249607: https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-August/039638.html

Therefore, relaxing loop strip mining verification is not an option.

The problem with the fix for JDK-8249607 is that it relies on IGVN being executed before pre/main/post loops are created, merging the two clones such that the remaining one has a safepoint use and is therefore correctly handled. However, this is not guaranteed. In fact, if major_progress is not set, pre/main/post loops could be created in the same round of loop opts without IGVN in-between (IdealLoopTree::iteration_split is executed right after PhaseIdealLoop::split_if_with_blocks).

I think we have the following options:

  1. Bail out if the loop is strip mined. I think that would be too conservative.

  2. Simply set major_progress when pinning a LoadNode to the OuterStripMinedLoop to make sure IGVN is executed and duplicate LoadNodes are merged. That seems quite invasive though.

  3. Allow the cloned load without a safepoint use (and therefore no usages in the OuterStripMinedLoop) to completely flow out of the loop. Currently, this is blocked by late_load_ctrl being computed on the initial LoadNode instead of the clone and therefore also taking into account the store that is referenced by the safepoint. We could simply re-compute the late ctrl for the cloned load, allowing it to flow out of the OuterStripMinedLoop. However, that only works if there is no anti-dependency in the OuterStripMinedLoop.

  4. Detect duplicate loads in the OuterStripMinedLoop on creation in PhaseIdealLoop::split_if_with_blocks and merge them right away.

I've decided to go with 4) and also reverted the fix for JDK-8249607 which is no longer required.

As Roland and Christian already noticed, there are various other issues with that code that hopefully will be addressed at some point by JDK-8252372.

Thanks,
Tobias


Progress

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

Issue

  • JDK-8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 29, 2021

👋 Welcome back thartmann! 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 label Jan 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@TobiHartmann 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
Copy link

@mlbridge mlbridge bot commented Jan 29, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

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

8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint

Reviewed-by: kvn, roland, 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 23 new commits pushed to the master branch:

  • ab727f0: 8260591: Shenandoah: improve parallelism for concurrent thread root scans
  • cf94208: 8259395: Patching automatic module with additional packages re-creates module without "requires java.base"
  • 039affc: 8260577: Unused code in AbstractCompiler after Shark compiler removal
  • 8a9004d: 8260574: Remove parallel constructs in GenCollectedHeap::process_roots
  • 0da9cad: 8260501: [Vector API] Improve register usage for shift operations on x86
  • a61ff87: 8260685: ProblemList 2 compiler/jvmci/compilerToVM tests in Xcomp configs
  • fcfe647: 8260462: Missing in Modality.html
  • 67a34da: 8260630: Templatize literal_size
  • 6b24e98: 8259008: ArithmeticException was thrown at "Monitor Cache Dump" on HSDB
  • 69ee314: 8249867: xml declaration is not followed by a newline
  • ... and 13 more: https://git.openjdk.java.net/jdk/compare/64a150c518ed1a936cd6e4cb5e60060243496901...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 label Jan 29, 2021
@TobiHartmann
Copy link
Member Author

@TobiHartmann TobiHartmann commented Feb 1, 2021

Thanks for the review, Vladimir!

@@ -1503,8 +1513,7 @@ void PhaseIdealLoop::split_if_with_blocks_post(Node *n) {
// to fold a StoreP and an AddP together (as part of an
// address expression) and the AddP and StoreP have
// different controls.
BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
if (!x->is_Load() && !x->is_DecodeNarrowPtr() && !x->is_AddP() && !bs->is_gc_barrier_node(x)) {
if (!x->is_Load() && !x->is_DecodeNarrowPtr()) {

This comment has been minimized.

@rwestrel

rwestrel Feb 1, 2021
Contributor

why this change?

This comment has been minimized.

@TobiHartmann

TobiHartmann Feb 1, 2021
Author Member

This reverts Christian's fix for JDK-8249607 which is no longer required.

This comment has been minimized.

@rwestrel

rwestrel Feb 1, 2021
Contributor

Ok.

@TobiHartmann
Copy link
Member Author

@TobiHartmann TobiHartmann commented Feb 1, 2021

Thanks for the review, Roland!

Copy link
Member

@chhagedorn chhagedorn left a comment

Nice summary! I agree that is better to go with 4) and try option 3) in a general clean up of this code (JDK-8252372). Looks good to me!

@TobiHartmann
Copy link
Member Author

@TobiHartmann TobiHartmann commented Feb 1, 2021

Thanks Christian!

@TobiHartmann
Copy link
Member Author

@TobiHartmann TobiHartmann commented Feb 2, 2021

/integrate

@openjdk openjdk bot closed this Feb 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 2, 2021

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

  • 474dba2: 8257086: Clarify differences between {Float, Double}.equals and ==
  • 54e7a64: 8260576: Typo in compiler/runtime/safepoints/TestRegisterRestoring.java
  • a6d9505: 8260864: ProblemList two security/krb5 tests on Linux
  • 9880c4c: 8260860: ProblemList tools/jlink/plugins/CompressorPluginTest.java
  • 55d62a5: 8213226: [TESTBUG] Reduce the usage of CDSTestUtils.executeAndLog()
  • b6a7367: 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS
  • 50f9a70: 8217327: G1 Post-Cleanup region liveness printing should not print out-of-date efficiency
  • e963ebd: 8260004: Shenandoah: Rename ShenandoahMarkCompact to ShenandoahFullGC
  • df33595: 8260309: Shenandoah: Clean up ShenandoahBarrierSet
  • 181d63f: 8260522: Clean up warnings in hotspot JTReg runtime tests
  • ... and 27 more: https://git.openjdk.java.net/jdk/compare/64a150c518ed1a936cd6e4cb5e60060243496901...master

Your commit was automatically rebased without conflicts.

Pushed as commit fe407cf.

💡 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
4 participants