-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8312570: [TESTBUG] Jtreg compiler/loopopts/superword/TestDependencyOffsets.java fails on 512-bit SVE #15010
Conversation
…fsets.java fails on 512-bit SVE Hotspot jtreg `compiler/loopopts/superword/TestDependencyOffsets.java` fails on AArch64 CPUs with 512-bit SVE. The reason is that many test loops in the code cannot be vectorized due to data dependence but IR tests assume they can. On AArch64, these IR tests just check the CPU feature of `asimd` and incorrectly assumes AArch64 vectors are at most 256 bits. But actually, `asimd` on AArch64 only represents NEON vectors which are at most 128 bits. AArch64 CPUs may have another feature of `sve` which represents scalable vectors of at most 2048 bits. The vectorization won't succeed on 512-bit SVE CPUs if the memory offset between some read and write is less than 512 bits. As this jtreg is auto-generated by a python script, we have updated the script and re-generated this jtreg. In this new version, we checked the auto-vectorization on both NEON-only and NEON+SVE platforms. Below is the diff of the generator script. We have also attached the new script to the JBS page. ``` @@ -321,7 +321,8 @@ class Type: p.append(Platform("avx512", ["avx512", "true"], 64)) else: assert False, "type not implemented" + self.name - p.append(Platform("asimd", ["asimd", "true"], 32)) + p.append(Platform("asimd", ["asimd", "true", "sve", "false"], 16)) + p.append(Platform("sve", ["sve", "true"], 256)) return p class Test: @@ -457,7 +458,7 @@ class Generator: lines.append(" * and various MaxVectorSize values, and +- AlignVector.") lines.append(" *") lines.append(" * Note: this test is auto-generated. Please modify / generate with script:") - lines.append(" * https://bugs.openjdk.org/browse/JDK-8308606") + lines.append(" * https://bugs.openjdk.org/browse/JDK-8312570") lines.append(" *") lines.append(" * Types: " + ", ".join([t.name for t in self.types])) lines.append(" * Offsets: " + ", ".join([str(o) for o in self.offsets])) @@ -598,7 +599,8 @@ class Generator: # IR rules for p in test.t.platforms(): elements = p.vector_width // test.t.size - lines.append(f" // CPU: {p.name} -> vector_width: {p.vector_width} -> elements in vector: {elements}") + max_pre = "max " if p.name == "sve" else "" + lines.append(f" // CPU: {p.name} -> {max_pre}vector_width: {p.vector_width} -> {max_pre}elements in vector: {elements}") ############### -AlignVector rule = PlatformIRRule(p) rule.add_pre_constraint("AlignVector", IRBool.makeFalse()) @@ -694,8 +696,8 @@ class Generator: def main(): g = Generator() g.generate("TestDependencyOffsets", - "/home/emanuel/Documents/fork7-jdk/open/test/hotspot/jtreg/compiler/loopopts/superword", - "8298935 8308606", # Big ID + "test/hotspot/jtreg/compiler/loopopts/superword", + "8298935 8308606 8312570", # Bug ID "compiler.loopopts.superword", # package ) ``` We tested this on various of AArch64 CPUs.
👋 Welcome back pli! A progress list of the required criteria for merging this PR into |
@eme64 Please help look at this. And, how about adding the test generator script you wrote into the jdk repo? |
Emanuel is on vacation until Aug 9. Can this wait or should we problem list? |
Thanks for the info. It's ok for us to wait, since very few people are using 512-bit SVE today. |
@pfustc @TobiHartmann I just saw this on my emails. So I'll give a quick response: We had this running on Aarch64 machines with What is the Adding the script to the jdk repo could be nice. But I think there may be some issues with adding python files. We may have to rewrite it in java. But I think for now adding and updating it via JBS is ok. |
Assuming NEON has larger vector size (256 bit, which is wrong) won't result in any failure on NEON-only machines. But it results in running less IR checks on 256-bit SVE. Let's take below IR condition change as an example.
Before this patch, the existence of vector IRs won't be checked on 256-bit SVE as we have
It just adds a prefix to make the comment more precise, as SVE uses scalable vectors and the vector length ranges from 128 bits to 2048 bits. |
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.
@pfustc Thanks for the changes and explanations, looks good to me! :)
Ah. Just one more idea: Since you now have even longer vector widths with 2048 bits: Should we not add some cases with even larger dependency offsets? We should go further than -196, 196
. We could consider adding 255, 256, 511, 512, 1024, 1536
(positive and negative). Of course the question is if that increases the runtime too much, what do you think?
@pfustc 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 no new commits pushed to 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.
@pfustc you will have to merge the changes from JDK-8310308. |
Thanks @eme64 @vnkozlov for review. Patch is currently merged with master. The python generator file is also updated and attached to the JBS page.
This is a good suggestion but I don't think it's necessary now and in the near future. Although Arm architecture document says SVE vector length can be at most 2048 bits, so far, the largest SVE vector length of real AArch64 CPUs in the world is 512 bits. AFAIK, there will not be real CPUs with 1024-bit SVE in the short term. So I prefer keeping current offset values for now. |
@pfustc ok, fair enough. If there is no real hardware, and not coming anytime soon, then we can leave the offsets as is. Thanks again for the fix! |
/integrate |
Going to push as commit e5ea9aa.
Your commit was automatically rebased without conflicts. |
Hotspot jtreg
compiler/loopopts/superword/TestDependencyOffsets.java
fails on AArch64 CPUs with 512-bit SVE. The reason is that many test loops in the code cannot be vectorized due to data dependence but IR tests assume they can.On AArch64, these IR tests just check the CPU feature of
asimd
and incorrectly assumes AArch64 vectors are at most 256 bits. But actually,asimd
on AArch64 only represents NEON vectors which are at most 128 bits. AArch64 CPUs may have another feature ofsve
which represents scalable vectors of at most 2048 bits. The vectorization won't succeed on 512-bit SVE CPUs if the memory offset between some read and write is less than 512 bits.As this jtreg is auto-generated by a python script, we have updated the script and re-generated this jtreg. In this new version, we checked the auto-vectorization on both NEON-only and NEON+SVE platforms. Below is the diff of the generator script. We have also attached the new script to the JBS page.
We tested this on various of AArch64 CPUs.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15010/head:pull/15010
$ git checkout pull/15010
Update a local copy of the PR:
$ git checkout pull/15010
$ git pull https://git.openjdk.org/jdk.git pull/15010/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15010
View PR using the GUI difftool:
$ git pr show -t 15010
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15010.diff
Webrev
Link to Webrev Comment