openjdk
8350177: C2 SuperWord: Integer.numberOfLeadingZeros, numberOfTrailingZeros, reverse and bitCount have input types wrongly turncated for byte and short
Hi all,
This patch fixes cases in SuperWord when compiling subword types where vectorized code would be given a narrower type than expected, leading to miscompilation due to truncation. This fix is a generalization of the same fix applied for Integer.reverseBytes in JDK-8305324. The patch introduces a check for nodes that are known to tolerate truncation, so that any future cases of subword truncation will avoid creating miscompiled code.
The patch reuses the existing logic to set the type of the vectors to int, which currently disables vectorization for the affected patterns entirely. Once JDK-8342095 is merged and automatic casting support is added the autovectorizer should automatically insert casts to and from int, maintaining correctness.
I've added an IR test that checks for correctly compiled outputs. Thoughts and reviews would be appreciated!
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8350177: C2 SuperWord: Integer.numberOfLeadingZeros, numberOfTrailingZeros, reverse and bitCount have input types wrongly turncated for byte and short (Bug - P3)(⚠️ The fixVersion in this issue is [25] but the fixVersion in .jcheck/conf is 26, a new backport will be created when this pr is integrated.)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25440/head:pull/25440
$ git checkout pull/25440
Update a local copy of the PR:
$ git checkout pull/25440
$ git pull https://git.openjdk.org/jdk.git pull/25440/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25440
View PR using the GUI difftool:
$ git pr show -t 25440
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25440.diff
Using Webrev
:wave: Welcome back jkarthikeyan! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@jaskarth 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:
8350177: C2 SuperWord: Integer.numberOfLeadingZeros, numberOfTrailingZeros, reverse and bitCount have input types wrongly truncated for byte and short
Reviewed-by: epeter, thartmann
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 306 new commits pushed to the master branch:
- bc828c8fb6693760c153a75188f96b1c9d201c8a: 6955128: Spec for javax.swing.plaf.basic.BasicTextUI.getVisibleEditorRect contains inappropriate wording
- 917d0182cb5ea6066afd396381ca4650371e64b0: 8361602: [TESTBUG] serviceability/HeapDump/UnmountedVThreadNativeMethodAtTop.java deadlocks on exception
- 3bacf7ea85f1e3f5e57fd2d046b98dfafe2c7e18: 8361869: Tests which call ThreadController should mark as /native
- ... and 303 more: https://git.openjdk.org/jdk/compare/e55ddabffa90e28d22f546b387007fe4e434c3e0...master
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.
@jaskarth 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.
Webrevs
- 08: Full - Incremental (893bebd9)
- 07: Full - Incremental (f779b9dd)
- 06: Full - Incremental (3701c534)
- 05: Full - Incremental (c219e38e)
- 04: Full - Incremental (edce2a12)
- 03: Full - Incremental (ce16e2de)
- 02: Full - Incremental (e2ab39c4)
- 01: Full - Incremental (da692994)
- 00: Full (8d1a8174)
@jaskarth Just to let you know, the fork is coming up this Thursday. But since this is a P3, we still got some time left in RDP 1 to get this fixed in JDK 25.
Thanks a lot for your review @eme64! I've pushed a commit that should address the reviews, and fix the GHA failures. I've added char tests as well. Regarding long operations, I don't think it's possible for this code path to encounter them. Earlier in the function, this logic is guarded with vtn->basic_type() == T_INT so I think only integer nodes need to be added to the list. Let me know what you think!
@chhagedorn Thanks for the reminder! It might be good to run some testing so that we can get it tested and reviewed before the RDP1 cutoff.
@eme64 Thank for taking another look! I've pushed a commit that adds an assert so that we can detect when unexpected nodes pass through the truncation filter. BTW, how did the testing go?
Since the base is before the branch off, could you please merge master before integration and see if GHA still passes? This also helps us testing libgraal compilation.
@eme64 I've updated the patch to address the comments, let me know what you think!
@mur47x111 Thanks for the comment! I've merged from master.
@jaskarth The code now looks great :) I'll run some testing over the weekend!
@jaskarth I just checked the results, and there is a series of failing tests.
compiler/c2/Test6958485.java
Flags: -server -Xcomp or -XX:+UnlockExperimentalVMOptions -XX:PerMethodSpecTrapLimit=0 -XX:PerMethodTrapLimit=0.
# assert(false) failed: Unexpected node in SuperWord truncation: Conv2B
compiler/intrinsics/TestDoubleIsInfinite.java -> D
compiler/intrinsics/TestFloatIsInfinite.java -> F
Flags: -XX:UseAVX=3 or -XX:-TieredCompilation -XX:+StressReflectiveCode -XX:-ReduceInitialCardMarks -XX:-ReduceBulkZeroing -XX:-ReduceFieldZeroing ... not sure if any are necessary actually.
# assert(false) failed: Unexpected node in SuperWord truncation: IsInfiniteD
and
# assert(false) failed: Unexpected node in SuperWord truncation: IsInfiniteF
jdk/incubator/vector/Byte128VectorTests.java (same issue with all related vector tests, just reporting one here)
Flag: -XX:UseAVX=2
# assert(false) failed: Unexpected node in SuperWord truncation: AddReductionVI
@eme64 Thanks for the test results! I've added these nodes to the non-truncating list, as well as the other reduction nodes that showed up when running the vector tests.
@jaskarth Thanks for the updates! :blush:
I'm running another round of testing, to see if we now handled all cases :)
@jaskarth the tests look better now. I still saw this failure:
jdk/incubator/vector/Byte64VectorTests.java
Flags: -XX:UseAVX=3 -XX:+UnlockDiagnosticVMOptions -XX:+UseKNLSetting or -XX:UseAVX=0 or -XX:UseAVX=2 ... probably no flags are actually required.
# assert(false) failed: Unexpected node in SuperWord truncation: ExtractB
@eme64 Thanks for running it again! I've pushed a fix for the ExtractB assert, and a proactive fix marking any nodes with TypeVect as their base type as non-truncating.
@TobiHartmann @eme64 Thanks a lot for the testing and re-reviews! I've fixed the PR title and will integrate it.
/integrate
Going to push as commit 77bd417c9990f57525257d9df89b9df4d7991461.
Since your change was applied there have been 306 commits pushed to the master branch:
- bc828c8fb6693760c153a75188f96b1c9d201c8a: 6955128: Spec for javax.swing.plaf.basic.BasicTextUI.getVisibleEditorRect contains inappropriate wording
- 917d0182cb5ea6066afd396381ca4650371e64b0: 8361602: [TESTBUG] serviceability/HeapDump/UnmountedVThreadNativeMethodAtTop.java deadlocks on exception
- 3bacf7ea85f1e3f5e57fd2d046b98dfafe2c7e18: 8361869: Tests which call ThreadController should mark as /native
- ... and 303 more: https://git.openjdk.org/jdk/compare/e55ddabffa90e28d22f546b387007fe4e434c3e0...master
Your commit was automatically rebased without conflicts.
@jaskarth Pushed as commit 77bd417c9990f57525257d9df89b9df4d7991461.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.