8255246: AArch64: Implement BigInteger shiftRight and shiftLeft accelerator/intrinsic #861
Conversation
…ftLeftImplWorker with NEON instructions
|
👋 Welcome back dongbo! A progress list of the required criteria for merging this PR into |
openjdk
bot
added
hotspot-compiler
Webrevs
|
|
Mailing list message from Andrew Haley on hotspot-compiler-dev: On 26/10/2020 09:46, Dong Bo wrote:
I don't see any performance testing here for shifts of small Thanks, -- |
|
@theRealAph Thanks for the quick review. Updated a version for small BigIntegers. According to the new tests, performance regress 3%~%6 only if The performance of The BigIntegers.java JMH micro-benchmark results of small BigIntegers (~256bits): |
|
Mailing list message from Andrew Haley on hotspot-compiler-dev: On 27/10/2020 06:47, Dong Bo wrote:
OK. I think there's no point pushing the small BigIntegers case any further This is the profile with maxNumbits = 256, and even then the cost of doing the StubRoutines::bigIntegerLeftShiftWorker [0x0000ffff80421d00, 0x0000ffff80421dd0] (208 bytes) -- |
| __ orrw(r12, r10, r11); | ||
| __ strw(r12, __ post(newArr, 4)); | ||
| __ sub(numIter, numIter, 1); | ||
| __ cbz(numIter, Exit); |
There was a problem hiding this comment.
This is odd code. Why not cbnz(numIter, ShiftOneLoop) ?
There was a problem hiding this comment.
My bad, it should be cbnz(numIter, ShiftOneLoop).
But it's gone now, the ShiftOneLoop is unrolled in the newest version.
Do you think we need further modifications?
|
@dgbo 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 49 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. 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 (@theRealAph) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@theRealAph Thanks for the review. @RealFYang Could you please sponsor this? Thanks. /integrate |
|
/sponsor |
openjdk
bot
added
integrated
openjdk
bot
removed
ready
|
@RealFYang @dgbo Since your change was applied there have been 49 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 6b2d11b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
BigInteger.shiftRightImplWorker and BigInteger.shiftLeftImplWorker are not intrinsified on aarch64, which have been done on x86_64.
We can implement them via USHL NEON instruction (register), which handles four integers one time at most, against just integer C2 asm-code processed.
The usage of USHL can be found at: https://developer.arm.com/documentation/dui0801/g/A64-SIMD-Vector-Instructions/USHL--vector-?lang=en
Patch passed jtreg tier1-3 tests on our aarch64 server.
Tests in test/jdk/java/math/BigInteger/* runned specially for the correctness of the implementation and passed.
We tested test/micro/org/openjdk/bench/java/math/BigIntegers.java for performance gain on Kunpeng916 and Kunpeng920.
The following performance improvements were seen with this implementation:
The BigIntegers.java JMH micro-benchmark results:
Progress
Testing
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/861/head:pull/861$ git checkout pull/861