8288564: C2: LShiftLNode::Ideal produces wrong result after JDK-8278114 #29
Conversation
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
@chhagedorn The following label will be automatically applied to this pull request:
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. |
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.
Good. Just one comment.
src/hotspot/share/opto/mulnode.cpp
Outdated
@@ -813,6 +813,7 @@ Node *LShiftINode::Ideal(PhaseGVN *phase, bool can_reshape) { | |||
// Left input is an add of the same number? | |||
if (add1->in(1) == add1->in(2)) { | |||
// Convert "(x + x) << c0" into "x << (c0 + 1)" | |||
assert(con != BitsPerJavaInteger - 1, "sanity check, optimization cannot be applied for con == 31"); |
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.
The assert is useless because there is check at line 812 `(con < 16).
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.
Yes, that's true. I just wanted to make sure that we do not forget about the fact that we cannot apply this optimization for con == 31
if this optimization is moved at some point or we decide to remove the con < 16
restriction. But I guess I can also just add a comment instead.
@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:
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
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
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.
Good
Thanks Vladimir and Igor for your reviews! |
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.
Nice analysis. Looks good.
Thanks Tobias for your review! /integrate |
Going to push as commit ed714af.
Your commit was automatically rebased without conflicts. |
@chhagedorn Pushed as commit ed714af. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
JDK-8278114 added the following transformation for integer and long left shifts:
However, in the long shift case, this transformation is not correct if
c0
is 63:while
which is not the same. For example, if
x = 1
:This optimization does not account for the fact that
x << 64
is the same asx << 0 = x
. According to the Java spec, chapter 15.19, we only consider the six lowest-order bits of the right-hand operand (i.e."right-hand operand" & 0b111111
). Therefore,x << 64
is the same asx << 0
(64 = 0b10000000 & 0b0111111 = 0
).Integer shifts are not affected because we do not apply this transformation if
c0 >= 16
:jdk19/src/hotspot/share/opto/mulnode.cpp
Lines 810 to 817 in 729164f
The fix I propose is to not apply this optimization for long left shifts if
c0 == 63
. I've added an additional sanity assertion for integer left shifts just in case this optimization is moved at some point and ending up outside the check forcon < 16
.Thanks,
Christian
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk19 pull/29/head:pull/29
$ git checkout pull/29
Update a local copy of the PR:
$ git checkout pull/29
$ git pull https://git.openjdk.org/jdk19 pull/29/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29
View PR using the GUI difftool:
$ git pr show -t 29
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/29.diff