8380753: C2: RegMask::find_first_set() incorrectly returns partially contained sets for scalable vector registers#30890
8380753: C2: RegMask::find_first_set() incorrectly returns partially contained sets for scalable vector registers#30890shqking wants to merge 1 commit into
Conversation
…contained sets for scalable vector registers Issue: compiler/loopopts/superword/TestAlignVectorFuzzer.java fails intermittently only on Neoverse-V1 machine (256-bit SVE) after JDK-8325467 with: ``` Internal Error (/tmp/jdk-src/src/hotspot/share/opto/chaitin.cpp:1451), pid=101028, tid=101050 assert(mask.can_represent(assigned)) failed: sanity ``` Root cause: On Neoverse-V1, the physical size of a scalable vector register exceeds RegMask::SlotsPerVecA (i.e. 2x larger). When querying a stack slot, RegMask::find_first_set() returns a register that is partially covered by the mask, violating can_represent(). Note-1: The failure is intermittent. I didn't check the jtreg code in details. I suppose the randomized and stress tests hit this corner case. Note-2: JDK-8325467 converted a previously handled can_represent() check into an assertion. I suppose the bug is independent of other parts of JDK-8325467. In this patch, we further validate the query result in RegMask::find_first_set(). Test-1: Run TestAlignVectorFuzzer.java case 100 times on Neoverse-V1 and all passes. Test-2: Run tier1, tier2 and tier3 on Linux AArch64 and x86_64. No new failure. Test-3: Add one unit test in gtest:RegMask. It fails on AArch64 if the change to regmask.cpp (that is done in this patch) is removed. Co-contributed-by: Daniel Lunden <daniel.lunden@oracle.com>
|
👋 Welcome back haosun! A progress list of the required criteria for merging this PR into |
|
@shqking 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 117 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. ➡️ To integrate this PR with the above commit message to the |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
|
/contributor add @dlunde |
|
@shqking |
|
More tests we have done:
I suppose this issue may affect RISC-V as well. I don't have the access to RISC-V. I was wondering if @RealFYang could help run some unit test? Thanks
|
|
@shqking : Sure. Let me try it on linux-riscv64 with big vectors. Thanks for the ping. |
Kindly remind that
|
|
Thanks for this @shqking! The changeset looks good, to be on the safe side let me (re-)run a bit of testing |
anton-seoane
left a comment
There was a problem hiding this comment.
Testing passed, code looks good. Thanks!
RealFYang
left a comment
There was a problem hiding this comment.
LGTM. My local test on linux-riscv64 w/wo big vectors is good.
|
Thanks for your reviews and tests. /integrate |
|
Going to push as commit b3f658d.
Your commit was automatically rebased without conflicts. |
Issue: compiler/loopopts/superword/TestAlignVectorFuzzer.java fails intermittently only on Neoverse-V1 machine (256-bit SVE) after JDK-8325467 with:
Root cause: On Neoverse-V1, the physical size of a scalable vector register exceeds RegMask::SlotsPerVecA (i.e. 2x larger). When querying a stack slot, RegMask::find_first_set() returns a register that is partially covered by the mask, violating can_represent().
Note-1: The failure is intermittent. I didn't check the jtreg code in details. I suppose the randomized and stress tests hit this corner case.
Note-2: JDK-8325467 converted a previously handled can_represent() check into an assertion. I suppose the bug is independent of other parts of JDK-8325467.
In this patch, we further validate the query result in RegMask::find_first_set().
Test-1: Run TestAlignVectorFuzzer.java case 100 times on Neoverse-V1 and all passes.
Test-2: Run tier1, tier2 and tier3 on Linux AArch64 and x86_64. No new failure.
Test-3: Add one unit test in gtest:RegMask. It fails on AArch64 if the change to regmask.cpp (that is done in this patch) is removed.
Co-contributed-by: Daniel Lunden daniel.lunden@oracle.com
Progress
Issue
Reviewers
Contributors
<dlunden@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30890/head:pull/30890$ git checkout pull/30890Update a local copy of the PR:
$ git checkout pull/30890$ git pull https://git.openjdk.org/jdk.git pull/30890/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30890View PR using the GUI difftool:
$ git pr show -t 30890Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30890.diff
Using Webrev
Link to Webrev Comment