-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8263303: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint #4278
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
Conversation
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable. Looks good to me!
// rr record ~/jdk-jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java -XX:-TieredCompilation -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:+PrintCompilation -XX:CompileOnly=TestPinnedUseInOuterLSM::test -XX:CompileCommand=quiet -XX:PrintIdealGraphFile=graph.xml -XX:PrintIdealGraphLevel=2 -XX:+UseG1GC -XX:+TraceLoopOpts -XX:LoopUnrollLimit=0 TestPinnedUseInOuterLSM | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a leftover, can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@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:
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 175 new commits pushed to the
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 |
@chhagedorn thanks for the review. |
Yes, we can't assume that all node in outer loop are referenced by Safepoint node. SP nodes reference only live values in current BC. In the test case it is value 'v'. After constant fold `v' become 0 and that is what SF should see (MulNode::Value() optimizes multiplication by 0). array[i] load should be dead at this point since nothing should be referencing it (except GC barriers, may be). And I don't get how array[i] is moved from inner loop - it depends on |
Thanks for looking at this @vnkozlov
The test case has: return v + array[i-1] + f;
It's sunk out of loop because it only has uses out of loop. One of the uses is still indirectly the safepoint at the time sinking happens so it's pinned in the outer loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me run tests before you push.
Got it. I assume it happens after constant folding
Okay. |
Testing results are good. |
@vnkozlov thanks for the review & testing |
/integrate |
@rwestrel Since your change was applied there have been 189 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit d4377af. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The loop strip mining verification code catches a node which is pinned
in the outer strip mined loop but not referenced from the safepoint
node.
The test case is an example of how this could happen. The array[i]
load is referenced from the safepoint initially through:
(j - 10) * array[i]
It is sunk out of the inner loop and pinned in the outer loop. A
following loop opts round causes j - 10 to constant fold to 0 and as a
consequence the array load is no longer referenced from the safepoint
but pinned in the outer strip mined loop.
I tried to find a way to preserve the invariant that all nodes in the
outer strip mined loop are referenced from the safepoint but found no
robust way for that. So the fix removes that part of the verification
code.
This requires loop cloning code to be adjusted for nodes pinned in the
outer strip mined loop but not referenced from the safepoint.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4278/head:pull/4278
$ git checkout pull/4278
Update a local copy of the PR:
$ git checkout pull/4278
$ git pull https://git.openjdk.java.net/jdk pull/4278/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4278
View PR using the GUI difftool:
$ git pr show -t 4278
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4278.diff