8289604: compiler/vectorapi/VectorLogicalOpIdentityTest.java failed on x86 AVX1 system

[XiaohongGong]

The sub-test "testMaskAndZero()" failed on x86 systems when
UseAVX=1 with the IR check failure:

  - counts: Graph contains wrong number of nodes:
     * Regex 1: (\\d+(\\s){2}(StoreVector.*)+(\\s){2}===.*)
        - Failed comparison: [found] 0 >= 1 [given]
        - No nodes matched!

The root cause is the VectorMask.fromArray/intoArray APIs
are not intrinsified when "UseAVX=1" for long type vectors
with following reasons:

1. The system supported max vector size is 128 bits for
   integral vector operations when "UseAVX=1".
2. The match rule of VectorLoadMaskNode/VectorStoreMaskNode
   are not supported for vectors with 2 elements (see [1]).

Note that VectorMask.fromArray() needs to be intrinsified
with "LoadVector+VectorLoadMask". And VectorMask.intoArray()
needs to be intrinsified with "VectorStoreMask+StoreVector".
Either "VectorStoreMask" or "StoreVector" not supported by the
compiler backend will forbit the relative API intrinsification.

Replacing the vector type from Long to other integral types
in the test case can fix the issue.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/x86.ad#L1861

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8289604: compiler/vectorapi/VectorLogicalOpIdentityTest.java failed on x86 AVX1 system

Reviewers

• Jie Fu (@DamonFool - Reviewer)
• @kristylee88 (no known github.com user name / role)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9373/head:pull/9373
$ git checkout pull/9373

Update a local copy of the PR:
$ git checkout pull/9373
$ git pull https://git.openjdk.org/jdk pull/9373/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9373

View PR using the GUI difftool:
$ git pr show -t 9373

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9373.diff

[bridgekeeper]

👋 Welcome back xgong! 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.

[XiaohongGong]

/label add hotspot-compiler

[openjdk]

@XiaohongGong
The hotspot-compiler label was successfully added.

[mlbridge]

Webrevs

• 00: Full (20596d4d)

[DamonFool]

Looks good to me.

[openjdk]

@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:

8289604: compiler/vectorapi/VectorLogicalOpIdentityTest.java failed on x86 AVX1 system

Reviewed-by: jiefu, kvn

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 9 new commits pushed to the master branch:

• 3515604: 8280481: Duplicated stubs to interpreter for static calls
• d48694d: 8283335: Add exists and readAttributesIfExists methods to FileSystemProvider
• c45d613: 8289687: [JVMCI] bug in HotSpotResolvedJavaMethodImpl.equals
• 77c3bbf: 8289617: Remove test/jdk/java/net/ServerSocket/ThreadStop.java
• a5934cd: 8289698: AArch64: Need to relativize extended_sp in frame
• fd1bb07: 8287603: Avoid redundant HashMap.containsKey calls in NimbusDefaults.getDerivedColor
• 4c997ba: 8289520: G1: Remove duplicate checks in G1BarrierSetC1::post_barrier
• 1b997db: 8289427: compiler/compilercontrol/jcmd/ClearDirectivesFileStackTest.java failed with null setting
• 688712f: 8289633: Forbid raw C-heap allocation functions in hotspot and fix findings

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[vnkozlov]

I submitted testing.

[vnkozlov]

Including @chhagedorn for discussion.

I was wondering why our testing with UseAVX=1 did not catch this issue (test passed).
But then I remember that IR framework skip testing if such flag is used by testing. It is not on whitelist:
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java#L104

We are adding more IR framework testing for vectors. I think it is important to test them with different CPU's features if needed.

#8999 added filter to run some sub-tests if CPU's feature is present/absent. But it relies on testing infrastructure to be executed on corresponding machine. It is not reliable (how many machines left with only AVX1).

I suggest to allow a test add flags to whitelist with which it allows to run. It (together with #8999 and @requires) will allow test's author to specify range of CPU features which can be used for test and make sure they will be run.

[XiaohongGong]

Thanks for looking at this fix @vnkozlov !

Including @chhagedorn for discussion.

I was wondering why our testing with UseAVX=1 did not catch this issue (test passed). But then I remember that IR framework skip testing if such flag is used by testing. It is not on whitelist: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java#L104

Right, testing with such vm options won't trigger the IR testing!

We are adding more IR framework testing for vectors. I think it is important to test them with different CPU's features if needed.

#8999 added filter to run some sub-tests if CPU's feature is present/absent. But it relies on testing infrastructure to be executed on corresponding machine. It is not reliable (how many machines left with only AVX1).

I suggest to allow a test add flags to whitelist with which it allows to run. It (together with #8999 and @requires) will allow test's author to specify range of CPU features which can be used for test and make sure they will be run.

Totally agree! #8999 adds the possibility to limit the test for specific CPU feature, which is convenient for the IR tests that relies on the CPU feature. Please also see one of the tests in the VectorLogicalOpIdentityTest.java here: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/vectorapi/VectorLogicalOpIdentityTest.java#L160

But for the check of common IRs which is not the CPU specific ones, testing with different vm options are needed. And we cannot use ApplyIfxxx for the architecture specific options such as UseAVX or UseSVE in common tests (i.e. using "UseSVE" on x64 systems makes issue). So I agree that adding the needed flags to whitelist is better.

[vnkozlov]

tier1-tier2 testing passed. But I am still waiting result of run with failed test on machine with AVX1 only (it is in scheduled state).

[vnkozlov]

Finally few runs with failed tests were executed on AVX1 machine and they passed.

[XiaohongGong]

Finally few runs with failed tests were executed on AVX1 machine and they passed.

Thanks for the testing!

[XiaohongGong]

/integrate

[openjdk]

Going to push as commit fafe8b3.
Since your change was applied there have been 9 commits pushed to the master branch:

• 3515604: 8280481: Duplicated stubs to interpreter for static calls
• d48694d: 8283335: Add exists and readAttributesIfExists methods to FileSystemProvider
• c45d613: 8289687: [JVMCI] bug in HotSpotResolvedJavaMethodImpl.equals
• 77c3bbf: 8289617: Remove test/jdk/java/net/ServerSocket/ThreadStop.java
• a5934cd: 8289698: AArch64: Need to relativize extended_sp in frame
• fd1bb07: 8287603: Avoid redundant HashMap.containsKey calls in NimbusDefaults.getDerivedColor
• 4c997ba: 8289520: G1: Remove duplicate checks in G1BarrierSetC1::post_barrier
• 1b997db: 8289427: compiler/compilercontrol/jcmd/ClearDirectivesFileStackTest.java failed with null setting
• 688712f: 8289633: Forbid raw C-heap allocation functions in hotspot and fix findings

Your commit was automatically rebased without conflicts.

[openjdk]

@XiaohongGong Pushed as commit fafe8b3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[chhagedorn]

Including @chhagedorn for discussion.

I was wondering why our testing with UseAVX=1 did not catch this issue (test passed). But then I remember that IR framework skip testing if such flag is used by testing. It is not on whitelist: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java#L104

We are adding more IR framework testing for vectors. I think it is important to test them with different CPU's features if needed.

#8999 added filter to run some sub-tests if CPU's feature is present/absent. But it relies on testing infrastructure to be executed on corresponding machine. It is not reliable (how many machines left with only AVX1).

I suggest to allow a test add flags to whitelist with which it allows to run. It (together with #8999 and @requires) will allow test's author to specify range of CPU features which can be used for test and make sure they will be run.

Right, UseAVX is not a whitelisted flag and thus IR matching is disabled. I think it would be a good idea to consider whitelisting this flag in general to allow (pre-integration) testing of such different default machine setups (we could be executing any test on such a machine at some point). Same for other flags such as UseSVE, UseSSE etc. I've filed JDK-8289801 to follow up on that. I will be on vacation starting from tomorrow. If someone wants to take this over, please go ahead. Otherwise, I'll come back to this when I'm back at the beginning of August.

Thanks,
Christian