-
Notifications
You must be signed in to change notification settings - Fork 113
8353182: [lworld] C2: Multiple IR test failures in compiler/gcbarriers/TestZGCBarrierElision.java after JDK-8351569 #1497
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
…riers/TestZGCBarrierElision.java after JDK-8351569
👋 Welcome back dfenacci! A progress list of the required criteria for merging this PR into |
@dafedafe 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 no new commits pushed to the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@dafedafe this pull request can not be integrated into git checkout JDK-8353182
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push |
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.
Thanks again Damon. Looks good to me!
Thanks for your review @TobiHartmann. |
/integrate |
/sponsor |
Going to push as commit ed2a03e.
Your commit was automatically rebased without conflicts. |
@TobiHartmann @dafedafe Pushed as commit ed2a03e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This is a very similar issue to JDK-8343420.
A few
TestZGCBarrierElision
tests fail after JDK-8351569. These tests check that ZGC barrier elision optimization does not elide necessary barriers and partially do so usingVarHandle::getAndSet
and checking thatGetAndSetP
nodes with barriers are created.As found out in JDK-8343420, JDK-8351569 refactored
Unsafe::getAndSet
(which is used byVarHandle::getAndSet
) to take value-classes into account, which implies that checks must be introduced to see if the target and argument objects are value-classes. This is done with a new overloadedgetAndSet
method that is now called instead of the old one. This method is not intrinsified (only inlined) and doesn't result in aGetAndSet
node. It instead relies on thecompareAndSet
method (which gets intrinsified) and results in aCompareAndSwap
node.Here as well, as the goal of the test is mainly to check for ZGC barrier presence, the sensible fix for now seems to be to modify the
TestZGCBarrierElision
tests to make them check forCompareAndSwap
instead ofGetAndSet
nodes.Tests: Tier 1-3+
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1497/head:pull/1497
$ git checkout pull/1497
Update a local copy of the PR:
$ git checkout pull/1497
$ git pull https://git.openjdk.org/valhalla.git pull/1497/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1497
View PR using the GUI difftool:
$ git pr show -t 1497
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1497.diff
Using Webrev
Link to Webrev Comment