8309894: compiler/vectorapi/VectorLogicalOpIdentityTest.java fails on SVE system with UseSVE=0 #14533
Conversation
… SVE system with UseSVE=0
|
👋 Welcome back xgong! A progress list of the required criteria for merging this PR into |
|
@XiaohongGong The following label will be automatically applied to this pull request:
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
|
|
@XiaohongGong Thanks for looking into this. But it seems to me this is not the same approach as we are taking with x86 SSE and AVX, where the jdk/src/hotspot/cpu/x86/vm_version_x86.cpp Lines 954 to 955 in 8d89992 Can you do a similar thing in It would be nice not to have to check for the flag and the features in every test, but just for the features. And the features should depend on what is present on the hardware, minus the restrictions by the flags. |
Thanks for looking at this PR @eme64 ! Yes, that's the main difference between aarch64 and x86 platforms. It actually makes things simpler that changing the CPU features based on the vm option. But per my understanding, CPU features are the hardware's feature which is the objective fact, while the
For each SVE test, we have tried to add flag For this test, we cannot add such an option in the test file, since it is also used to test other platforms like x86. |
|
@XiaohongGong I see, you are worried that it would take a lot of work in the Are you going to add the I just really don't like the duplication of IR rules, and the checking of both the flag and the cpu feature. Another solution: we filter out the |
That is the question: what is the ground truth. On x86, the idea is to keep the flags and the cpu features in sync. So if the hardware does not support a flag value to be larger (eg trying to set Of course, the hardware may support more features than the VM assumes. But still, that allows us to do some "cross cpu" testing - we can simulate avx1 features on a avx2 machine for example. I discussed it with @chhagedorn and @TobiHartmann . They also think it is better to have flags and cpu features in sync. Because the flags really should restrict what the VM uses everywhere. We want to make sure the restrictions apply in the VM, if we check for features or flags. |
|
@fg1417 @jatin-bhateja What do you think about the consistency of arm / intel hardware flags for |
Yes, that may need more overhead on AArch64 code. I think the changing should not only limit to And yes, for the current backend, we're almost using
I'm afraid we have to modify all these tests, and what I know is my colleague @pfustc will take charge of the vectorization tests once this PR is merged. Besides, we have to only care about the rules that
Thanks for the advice! Yeah, this seems a workaround. Besides, adding such specific handling for |
Hi @eme64, Having flags and cpu features in sync sounds a good idea. However, of all platforms supported by HotSpot, only x86 does in this way. Even on x86, only AVX & SSE related features have such sync at present. There is no sync for other feature strings and flags, like |
It might, but it sounds like it's the right thing to do as soon as possible after the JDK 21 fork. |
I agree.
That's an interesting suggestion, but having to keep all of the back ends in lock-step is an intolerable constraint. |
Thanks for looking at this issue @theRealAph. Sounds a good suggestion! I will take an investigation for this, and try to make a change. |
|
Hi there, I'v filed the vm option and cpu feature sync issue here for AArch64: https://bugs.openjdk.org/browse/JDK-8311130, and will address the comment with it. Thanks again for the advice! Hi @eme64 , besides the sync issue, does the change to IR framework make sense to you? Currently, if we use an architecture specific vm options with If that part seems fine to you, maybe we can let this PR in first? Since the test failure will noise our internal ci testing. WDYT? Thanks! |
|
@XiaohongGong I totally agree with the changes to the IR framework (having Otherwise, using both I was a bit afraid not keeping the CPU feature and the VM flag in sync could also lead to issues in the backend of aarch64. But it does indeed seem that we only use Actually, since there are only so few uses of Anyway, I just launched testing for commit 1: tier1-6 plus stress testing. Will report back on Monday probably. |
Thanks a lot!
We (Arm) will do the real fix.
Agree, although we only use
I prefer fixing that in a separate patch. One reason is syncing the vm options and cpu features is a refactory to AArch64 backend for me. It has other relative cpu features specific to different SVE systems besides And besides the
Thanks for doing this! |
|
@XiaohongGong Testing for commit 1 looks good. I don't have the expertise on the other CPU features. But from a distance, I'd say yes. The idea with all the CPU features is that we can simulate less powerful machines with more powerful machines. But I would first fix the SVE features, and handle the others separately. Maybe for those a separate discussion needs to happen first. |
There was a problem hiding this comment.
Accepted as a temporary fix that has to be reverted with JDK-8311130.
Thanks again for fixing the order of CPU features and VM flags in the IR rules!
|
@XiaohongGong 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 269 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 |
Thanks for the review and testing! I will revert the IR test part with JDK-8311130. |
There was a problem hiding this comment.
Accepted as a temporary fix that has to be reverted with JDK-8311130.
I agree with this temporary fix and reverting it again when syncing the flags with the CPU features in JDK-8311130.
test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPreconditions.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review @chhagedorn ! I'v addressed the comments in latest commit. |
|
/integrate |
|
Going to push as commit 60544f9.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@XiaohongGong Pushed as commit 60544f9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
As discussed in PR openjdk#14533, keeping AArch64 flag `UseSVE` and its related CPU features in sync helps to simplify rules in IR tests. In this patch, we mask SVE related CPU features off if specified SVE level in VM option is lower than the hardware supported. Also, to support this change, we move the features string construction to the end of the `initialize()` function. We also revert IR rule changes in PR openjdk#14533 and fix some code styles. We tested almost full jtreg on SVE, SVE2 and non-SVE CPUs and no new issue is found after this patch.
This test fails with several IR check failures when run on ARM SVE systems with vm option
-XX:UseSVE=0. Here is one of the failed IR rule:The specified IR in the test depends on the platform's predicate feature. Hence the IR check can be applied only on ARM SVE or X86 AVX512 systems. But with
-XX:UseSVE=0on ARM SVE machines, JVM will disable SVE feature for compiler. But the CPU feature is not changed. To guarantee the IR rule is run with SVE as expected, it has to add another condition likeapplyIf = {"UseSVE", ">0"}. ConsiderUseSVEis an ARM specific option, it can be used only on ARM CPUs. So the right IR rules should be:This patch also changes the check order of conditions for a IR rule. It's better to check
applyIfCPUFeaturebeforeapplyIf, in case the vm option is invalid on running hardware, which makes test throw exception and abort.Verified on X86 systems with
UseAVX=1/2/3by removing the test from ProblemList.txt, and SVE systems withUseSVE=0/1.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14533/head:pull/14533$ git checkout pull/14533Update a local copy of the PR:
$ git checkout pull/14533$ git pull https://git.openjdk.org/jdk.git pull/14533/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14533View PR using the GUI difftool:
$ git pr show -t 14533Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14533.diff
Webrev
Link to Webrev Comment