8320897: compiler/vectorapi/reshape/TestVectorReinterpret.java fails on ppc64(le) platforms

[TheRealMDoerr]

testB64toB128 and testB128toB64 are using 8 Byte vector operations which are available for some platforms, but not for ppc64 and s390 which currently only support 16 Byte vector operations by default:

  ** Rejected vector op (VectorReinterpret,byte,8) because architecture does not support it
  ** not supported: arity=1 op=reinterpret/1 vlen1=8 etype1=byte ismask=0

See JBS comment for complete output.
Similar for TestVectorRebracket methods.

---

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

Issues

• JDK-8320897: compiler/vectorapi/reshape/TestVectorReinterpret.java fails on ppc64(le) platforms (Bug - P4)
• JDK-8348519: [s390x] test failure TestVectorReinterpret.java (Bug - P4)

Reviewers

• Amit Kumar (@offamitkumar - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30554

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@TheRealMDoerr 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.

[TheRealMDoerr]

@offamitkumar: You may want to check if this solves JDK-8348519, too.

[mlbridge]

Webrevs

• 01: Full - Incremental (bc9ca1f1)
• 00: Full (e0b14eb9)

[offamitkumar]

@offamitkumar: You may want to check if this solves JDK-8348519, too.

Hi Martin, yes, failure is not anymore there, test is disabled for s390.

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR  SKIP   
   jtreg:test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorReinterpret.java
                                                         0     0     0     0     0   
==============================
TEST SUCCESS

[TheRealMDoerr]

@offamitkumar: You may want to check if this solves JDK-8348519, too.

Hi Martin, yes, failure is not anymore there, test is disabled for s390.

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR  SKIP   
   jtreg:test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorReinterpret.java
                                                         0     0     0     0     0   
==============================
TEST SUCCESS

I mean if it passes when you remove it from the ProblemList:

diff --git a/test/hotspot/jtreg/ProblemList.txt b/test/hotspot/jtreg/ProblemList.txt
index d4efaa7e631..b4f9efff830 100644
--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -56,7 +56,6 @@ compiler/c2/irTests/TestDuplicateBackedge.java 8318904 generic-all
 
 compiler/codecache/jmx/PoolsIndependenceTest.java 8264632 macosx-all
 
-compiler/vectorapi/reshape/TestVectorReinterpret.java 8348519 linux-s390x
 compiler/vectorapi/VectorRebracket128Test.java 8330538 generic-all
 
 compiler/vectorization/TestVectorAlgorithms.java#noSuperWord 8376803 aix-ppc64,linux-s390x

[offamitkumar]

Hi Martin,
Yes test passes on s390x;

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR  SKIP   
   jtreg:test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorReinterpret.java
                                                         1     1     0     0     0   
==============================
TEST SUCCESS

[TheRealMDoerr]

/issue add JDK-8320897

[openjdk]

@TheRealMDoerr This issue is referenced in the PR title - it will now be updated.

[TheRealMDoerr]

Hi Martin, Yes test passes on s390x;

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR  SKIP   
   jtreg:test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorReinterpret.java
                                                         1     1     0     0     0   
==============================
TEST SUCCESS

Thanks for checking! I have added the Problem List update to this PR. A review would be nice.

[TheRealMDoerr]

/issue add JDK-8348519

[openjdk]

@TheRealMDoerr
Adding additional issue to issue list: 8348519: [s390x] test failure TestVectorReinterpret.java.

[offamitkumar]

Thanks for fixing it. LGTM.

[reinrich]

This means that c2 does not use vector intrinsics on ppc64, right? Would c2 be able to use the intrinsics with -XX:-SuperwordUseVSX? E.g. are there vector instructions for, e.g., ByteVector.SPECIES_64?

[TheRealMDoerr]

This means that c2 does not use vector intrinsics on ppc64, right? Would c2 be able to use the intrinsics with -XX:-SuperwordUseVSX? E.g. are there vector instructions for, e.g., ByteVector.SPECIES_64?

This means that c2 does not use 8 Byte vector intrinsics for Reinterpret operations if SuperwordUseVSX is enabled (default on Power9 and later). In this mode, only 16 Byte VectorRegisters are used by the intrinsics which are passing the IR checks for non-8 Byte Reinterpret operations in this test.
With -XX:-SuperwordUseVSX, c2 only uses 8 Byte GP registers and the test passes, because everything is done in 8 Byte hunks. I guess this case is not particularly interesting regarding IR checks because 8 Byte Registers are used for everything, anyways.

[reinrich]

Are these test a type conversion and at runtime basically a nop?

Would c2 be able to use the intrinsics with -XX:-SuperwordUseVSX? E.g. are there vector instructions for, e.g., ByteVector.SPECIES_64?

I meant this as a more general question. I'd like to understand if there are instructions (besides load/store) for operations on ByteVector.SPECIES_64 vector data and if we miss opportunities for optimization if SuperwordUseVSX is enabled.

[TheRealMDoerr]

Are these test a type conversion and at runtime basically a nop?

Yes. At least as long as the result uses the same vector register type as the input.

Would c2 be able to use the intrinsics with -XX:-SuperwordUseVSX? E.g. are there vector instructions for, e.g., ByteVector.SPECIES_64?

I meant this as a more general question. I'd like to understand if there are instructions (besides load/store) for operations on ByteVector.SPECIES_64 vector data and if we miss opportunities for optimization if SuperwordUseVSX is enabled.

I'd like to understand that, too ;-)
Note that Vector API is not officially ported to PPC: https://wiki.openjdk.org/spaces/PPCAIXPort/overview
We only have a set of nodes which make it usable to some degree and the JTREG tests passing.
So, there is potentially room for improvement, but nobody has worked on it. I noticed that VectorMask nodes are implemented for other platforms. They may be related.

[reinrich]

Why are the IR checks failing for testB64toB128 while IR checks for testB64toB256 succeed?

[TheRealMDoerr]

Why are the IR checks failing for testB64toB128 while IR checks for testB64toB256 succeed?

Seems like only testB64toB128 and testB128toB64 use 8 Byte Reinterpret nodes. The 16 Byte ones work as expected.

[reinrich]

Why are the IR checks failing for testB64toB128 while IR checks for testB64toB256 succeed?

Seems like only testB64toB128 and testB128toB64 use 8 Byte Reinterpret nodes. The 16 Byte ones work as expected.

From the name testB64toB256 I'd expect that 8 byte Reinterpret nodes are needed. What type of Reinterpret nodes are used there?

[TheRealMDoerr]

Why are the IR checks failing for testB64toB128 while IR checks for testB64toB256 succeed?

Seems like only testB64toB128 and testB128toB64 use 8 Byte Reinterpret nodes. The 16 Byte ones work as expected.

From the name testB64toB256 I'd expect that 8 byte Reinterpret nodes are needed. What type of Reinterpret nodes are used there?

reinterpretX on PPC64 (with SuperwordUseVSX) AFAICS was my first assumption (vecX is a 16 Byte vector), but that came from another subtest. Not sure what happens with it.

[reinrich]

Looks like testB64toB256 is just not run. At least the TestVectorReinterpret.jtr doesn't mention it.

I assume this is because of the length filtering here:

jdk/test/hotspot/jtreg/compiler/vectorapi/reshape/utils/VectorReshapeHelper.java

Lines 83 to 84 in dc81630

	.filter(p -> p.isp().length() <= VectorSpecies.ofLargestShape(p.isp().elementType()).length())
	.filter(p -> p.osp().length() <= VectorSpecies.ofLargestShape(p.osp().elementType()).length())

reinterpretX on ppc64 is very much an identitiy operation (with an empty encoding). Other platforms have that too but there it goes with a pridicate like the following:

jdk/src/hotspot/cpu/aarch64/aarch64_vector.ad

Line 4446 in dc81630

predicate(Matcher::vector_length_in_bytes(n) == Matcher::vector_length_in_bytes(n->in(1)));

On ppc64 the predicate is missing. Maybe that's ok because we only support vecX.