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

8271340: Crash PhaseIdealLoop::clone_outer_loop #5220

Closed
wants to merge 4 commits into from

Conversation

@rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Aug 23, 2021

The test has a counted loop and an infinite loop. The infinite loop is
reachable from multiple paths and its loop head is a Region with more
than 3 inputs. One of these paths is from the counted loop. When loop
opts run, a NeverBranch is added to the infinite loop that's removed
by NeverBranchNode::Ideal() next because in(0) of the NeverBranch is a
Region and not a Loop.

When CCP runs, it finds the counted loop exit is never reached because
a test in the loop body that depends on a loop phi is never taken. As
a consequence nodes along the path from the counted loop end to the
infinite loop Region have type top. One of these nodes is a Call
node. PhaseCCP::do_transform() should then cause the path between the
counted loop and the infinite loop to optimize out but that doesn't
happen because do_transform() starts from Root by following inputs.
The dead path is only reachable from the infinite loop but the there's
no edge between Root and the infinite loop.

IGVN next runs, processes the Call Node, finds it's dead, kills
everything around it which causes the OuterStripMinedLoopEnd to loose
a projection. That later triggers the crash.

The fix I propose is to be more conservative in
NeverBranchNode::Ideal() and to check for a in(0) that's a Region. As
a consequence, at CCP time, the infinite loop is reachable from Root.

This change also requires some adjustments to Shenandoah specific code
that makes assumptions about the shape of infinite loops.

/cc hotspot-compiler


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5220

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 23, 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

@rwestrel
The hotspot-compiler label was successfully added.

Loading

@openjdk openjdk bot added the rfr label Aug 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 23, 2021

Webrevs

Loading

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Aug 31, 2021

Anyone for this?

Loading

@@ -2717,7 +2717,7 @@ const Type* NeverBranchNode::Value(PhaseGVN* phase) const {
//------------------------------Ideal------------------------------------------
// Check for no longer being part of a loop
Node *NeverBranchNode::Ideal(PhaseGVN *phase, bool can_reshape) {
if (can_reshape && !in(0)->is_Loop()) {
if (can_reshape && !in(0)->is_Region()) {
Copy link
Contributor

@vnkozlov vnkozlov Aug 31, 2021

Choose a reason for hiding this comment

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

I assume this is the actual fix.

Loading

Copy link
Contributor Author

@rwestrel rwestrel Aug 31, 2021

Choose a reason for hiding this comment

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

Yes, it is. The rest is shenandoah specific.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Sep 2, 2021

Choose a reason for hiding this comment

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

okay

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Please, place TestInfiniteLoopCCP.java test into compiler/loopopts

Loading

Copy link
Member

@TobiHartmann TobiHartmann left a comment

but that doesn't happen because do_transform() starts from Root by following inputs

Would the following code I added to Valhalla help?

openjdk/valhalla@f43fafc#diff-f4a11a2c3f7d7342641c878277f778856ad329e4e09026e39d08afb03efd10a2R1958

Loading

// Give thread some time to trigger compilation
thread.setDaemon(true);
thread.start();
Thread.sleep(/*Utils.adjustTimeout(4000)*/4000);
Copy link
Member

@TobiHartmann TobiHartmann Sep 2, 2021

Choose a reason for hiding this comment

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

Why is the Utils.adjustTimeout uncommented?

Loading

Copy link
Contributor Author

@rwestrel rwestrel Sep 3, 2021

Choose a reason for hiding this comment

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

That was an accident. Fixed in updated change.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.
But, please, address Tobias's question about test.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 2, 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:

8271340: Crash PhaseIdealLoop::clone_outer_loop

Reviewed-by: kvn, thartmann

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

  • fa9c865: 8273112: -Xloggc: should override -verbose:gc
  • dd87181: 8214761: Bug in parallel Kahan summation implementation
  • 7fff22a: 8272805: Avoid looking up standard charsets
  • 92b05fe: 8273251: Call check_possible_safepoint() from SafepointMechanism::process_if_requested()
  • 29e0f13: 8272385: Enforce ECPrivateKey d value to be in the range [1, n-1] for SunEC provider
  • aaa6f69: 8273250: Address javadoc issues in Deflater::setDictionationary
  • 5ee5dd9: 8272914: Create hotspot:tier2 and hotspot:tier3 test groups

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

Loading

@openjdk openjdk bot added the ready label Sep 2, 2021
@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Sep 3, 2021

Please, place TestInfiniteLoopCCP.java test into compiler/loopopts

fixed in the update change

Loading

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Sep 3, 2021

Would the following code I added to Valhalla help?

openjdk/valhalla@f43fafc#diff-f4a11a2c3f7d7342641c878277f778856ad329e4e09026e39d08afb03efd10a2R1958

I'm not sure. How would it help in this particular case?

In any case, it seems to me NeverBranchNode::Ideal() is simply wrong. As I understand, when loop opts hit an infinite loop it creates a NeverBranchNode but because the infinite loop is not properly registered in the loop tree, the loop's Region is not transformed into a Loop node. On the next pass of loop opts, if the NeverBranch is still in the infinite loop, a Loop is created for the infinite loop. But if NeverBranchNode::Ideal() runs in between then a new NeverBranch is added and maybe and only in some later loop opts pass a Loop created. So AFAICT, the behavior is inconsistent depending on whether NeverBranchNode::Ideal() runs or not which doesn't seem right.

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Sep 3, 2021

Would the following code I added to Valhalla help?
openjdk/valhalla@f43fafc#diff-f4a11a2c3f7d7342641c878277f778856ad329e4e09026e39d08afb03efd10a2R1958

I'm not sure. How would it help in this particular case?

I thought it might help by aggressively removing the parts not reachable from the bottom (but still reachable from root).

In any case, it seems to me NeverBranchNode::Ideal() is simply wrong. As I understand, when loop opts hit an infinite loop it creates a NeverBranchNode but because the infinite loop is not properly registered in the loop tree, the loop's Region is not transformed into a Loop node. On the next pass of loop opts, if the NeverBranch is still in the infinite loop, a Loop is created for the infinite loop. But if NeverBranchNode::Ideal() runs in between then a new NeverBranch is added and maybe and only in some later loop opts pass a Loop created. So AFAICT, the behavior is inconsistent depending on whether NeverBranchNode::Ideal() runs or not which doesn't seem right.

Thanks for the details. That makes sense.

Loading

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Sep 7, 2021

Thanks for the reviews @vnkozlov @TobiHartmann

Loading

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Sep 7, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 7, 2021

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

  • 99fb12c: 8271341: Opcode() != Op_If && Opcode() != Op_RangeCheck) || outcnt() == 2 assert failure with Test7179138_1.java
  • 041ae20: 8268558: [TESTBUG] Case 2 in TestP11KeyFactoryGetRSAKeySpec is skipped
  • 377b186: 8269119: C2: Avoid redundant memory barriers in Unsafe.copyMemory0 intrinsic
  • 70157c7: 8272135: jshell: Method cannot use its overloaded version
  • 5caa77b: 8263364: sun/net/www/http/KeepAliveStream/KeepAliveStreamCloseWithWrongContentLength.java wedged in getInputStream
  • a522d6b: 8273376: Zero: Disable vtable/itableStub gtests
  • 3cd95a2: 8231356: Fix broken ResourceObj::operator new[] in debug builds
  • 81c719b: 8273333: Zero should warn about unimplemented -XX:+LogTouchedMethods
  • 649c22c: 8270832: Aarch64: Update algorithm annotations for MacroAssembler::fill_words
  • eb22181: 8273386: Remove duplicated code in G1DCQS::abandon_completed_buffers
  • ... and 28 more: https://git.openjdk.java.net/jdk/compare/5245c1cf0260a78ca5f8ab4e7d13107f21faf071...master

Your commit was automatically rebased without conflicts.

Loading

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

@openjdk openjdk bot commented Sep 7, 2021

@rwestrel Pushed as commit 2abf3b3.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants