8257182: JCK test failures in integer / long rotation tests #1562

chhagedorn · 2020-12-02T14:04:54Z

The RotateLeftNode::Value() and RotateRightNode::Value() methods use an arithmetic instead of a logical shift due to applying the shift operator >> to a signed instead of an unsigned number. When, for example, rotating a negative even integer (least significant bit is 0) to the right, we should always get a positive number (most significant bit becoming 0). However, the arithmetic shift keeps the integer being negative. This lets some JCK rotation tests fail with -Xcomp.

The previously failing JCK tests work again with -Xcomp by applying this fix.

Thanks,
Christian

Progress

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

Issue

JDK-8257182: JCK test failures in integer / long rotation tests

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer)
Vladimir Ivanov (@iwanowww - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Download

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

bridgekeeper · 2020-12-02T14:05:50Z

👋 Welcome back chagedorn! 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 · 2020-12-02T14:06:53Z

@chhagedorn 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 · 2020-12-02T14:10:08Z

Webrevs

01: Full - Incremental (d907f75)
00: Full (b68a649)

TheRealMDoerr

Looks great! Thanks for fixing it and adding a regression test.

TheRealMDoerr · 2020-12-02T15:44:31Z

src/hotspot/share/opto/mulnode.cpp

-      int shift = r2->get_con() & (BitsPerJavaInteger - 1); // semantics of Java shifts
-      return TypeInt::make((r1->get_con() << shift) | (r1->get_con() >> (32 - shift)));
+      unsigned int r1_con = (unsigned int)r1->get_con();
+      unsigned int shift = (unsigned int)(r2->get_con()) & (unsigned int)(BitsPerJavaInteger - 1); // semantics of Java shifts


I'd prefer to use juint which actually is the same, but would be more consistent IMHO.

I agree, I'll change it to juint

chhagedorn · 2020-12-03T10:53:18Z

@TheRealMDoerr @vnkozlov Thanks for your reviews!

iwanowww

Looks good.

openjdk · 2020-12-03T12:51:10Z

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

8257182: JCK test failures in integer / long rotation tests

Reviewed-by: mdoerr, vlivanov, thartmann, kvn

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

6c9482e: 8257561: Some code is not vectorized after 8251925 and 8250607
70517c8: 8257642: CipherByteBufferOverwriteTest copyright issue
e836396: 8257436: [aarch64] Regressions in ArrayCopyUnalignedDst.testByte/testChar for 65-78 bytes when UseSIMDForMemoryOps is on
b170c83: 8257591: Remove suppression of record preview related warnings in java.lang
66a2e70: 8255845: Memory leak in imageFile.cpp
fa58671: 8257020: [JVMCI] enable a JVMCICompiler to specify which GCs it supports
129c377: 8257594: C2 compiled checkcast of non-null object triggers endless deoptimization/recompilation cycle
e4497c9: 8256718: Obsolete the long term deprecated and aliased Trace flags

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.

chhagedorn · 2020-12-03T12:54:54Z

@iwanowww Thanks for your review!

TobiHartmann

Looks good.

chhagedorn · 2020-12-04T08:09:36Z

@TobiHartmann @vnkozlov @TheRealMDoerr Thanks for your reviews!

chhagedorn · 2020-12-04T08:10:01Z

/integrate

openjdk · 2020-12-04T08:11:06Z

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

f33808f: 8257631: C2: Assertion failure in ArrayCopyNode::get_count() during late inlining
417e7e6: 8257632: C2: Late inlining attempt on a call with a dead memory crashes
6845fee: 8257625: C2: Harden input checks in vector intrinsics
4390f2c: 8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes
d08c612: 8257708: Remove redundant unmodifiableSet wrapper from already immutable set returned by Collections.singleton
49f9e57: 8257734: Extraneous output in HmacSHA3_512 constructor
f83fd4a: 8257438: Avoid adding duplicate values into extendedKeyCodesSet
37c7835: 8257673: Build fails without shenandoahgc after JDK-8257563
f0b1194: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
36209b7: 8257547: Handle multiple prereqs on the same line in deps files
... and 16 more: https://git.openjdk.java.net/jdk/compare/4a267f1bc2b025aae2cb9df7283156aeb0282406...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4a85514.

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

8257182: JCK test failures in integer / long rotation tests

b68a649

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2020

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 2, 2020

TheRealMDoerr reviewed Dec 2, 2020

View reviewed changes

chhagedorn added 2 commits December 3, 2020 11:58

Change unsigned int to juint

8140f97

Merge branch 'master' into JDK-8257182

d907f75

iwanowww approved these changes Dec 3, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2020

TobiHartmann approved these changes Dec 3, 2020

View reviewed changes

TheRealMDoerr approved these changes Dec 3, 2020

View reviewed changes

vnkozlov approved these changes Dec 3, 2020

View reviewed changes

openjdk bot closed this Dec 4, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 4, 2020

chhagedorn deleted the JDK-8257182 branch December 9, 2020 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8257182: JCK test failures in integer / long rotation tests #1562

8257182: JCK test failures in integer / long rotation tests #1562

chhagedorn commented Dec 2, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

mlbridge bot commented Dec 2, 2020 •

edited

Loading

TheRealMDoerr left a comment

TheRealMDoerr Dec 2, 2020

vnkozlov Dec 2, 2020

chhagedorn Dec 3, 2020

chhagedorn commented Dec 3, 2020

iwanowww left a comment

openjdk bot commented Dec 3, 2020 •

edited

Loading

chhagedorn commented Dec 3, 2020

TobiHartmann left a comment

chhagedorn commented Dec 4, 2020

chhagedorn commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

8257182: JCK test failures in integer / long rotation tests #1562

8257182: JCK test failures in integer / long rotation tests #1562

Conversation

chhagedorn commented Dec 2, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

mlbridge bot commented Dec 2, 2020 • edited Loading

Webrevs

TheRealMDoerr left a comment

Choose a reason for hiding this comment

TheRealMDoerr Dec 2, 2020

Choose a reason for hiding this comment

vnkozlov Dec 2, 2020

Choose a reason for hiding this comment

chhagedorn Dec 3, 2020

Choose a reason for hiding this comment

chhagedorn commented Dec 3, 2020

iwanowww left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 3, 2020 • edited Loading

chhagedorn commented Dec 3, 2020

TobiHartmann left a comment

Choose a reason for hiding this comment

chhagedorn commented Dec 4, 2020

chhagedorn commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

chhagedorn commented Dec 2, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 2, 2020 •

edited

Loading

openjdk bot commented Dec 3, 2020 •

edited

Loading