RFR: 8259610: VectorReshapeTests are not effective due to failing to intrinsify "VectorSupport.convert" [v4]

[Paul Sandoz]

On Thu, 9 Dec 2021 10:52:36 GMT, Mai Đặng Quân Anh <duke at openjdk.java.net> wrote:

>> Hi,
>> 
>> This patch adds several c2 tests for vector reshape operations. The tests verify the intrinsification of the corresponding operations by using the IR framework and verify the correctness of the results of compiled codes.
>> 
>> While working on this patch, I spot some regressions regarding compilation on AVX1.
>> 
>> Thank you very much.
>
> Mai Đặng Quân Anh has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix cpu pattern on Neon

Very nice work. Just a few comments.

test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorCastAVX1.java line 50:

> 48:         String testMethods = String.join(",", TestCastMethods.AVX1_CAST_TESTS.stream()
> 49:                 .map(VectorSpeciesPair::format)
> 50:                 .toList());

Use a joining collector:
Suggestion:

        String testMethods = TestCastMethods.AVX1_CAST_TESTS.stream()
                .map(VectorSpeciesPair::format)
                .collect(joining(","));


There is also a fair bit of repetition in each Test class, consider an abstract class with static method accepting the test class, additional flags, and test cases list? That would be more applicable if three general cases in `TestVectorReinterpret` were separated out, or at least the expand tests from the rebracket tests.

test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorReinterpret.java line 48:

> 46:     private static final List<VectorShape> SHAPE_LIST = List.of(
> 47:             VectorShape.S_64_BIT, VectorShape.S_128_BIT, VectorShape.S_256_BIT, VectorShape.S_512_BIT
> 48:     );

Suggestion:

    private static final List<VectorShape> SHAPE_LIST = List.of(VectorShape.values());

test/hotspot/jtreg/compiler/vectorapi/reshape/TestVectorReinterpret.java line 58:

> 56:         expandShrink.addHelperClasses(VectorReshapeHelper.class);
> 57:         expandShrink.addFlags("--add-modules=jdk.incubator.vector");
> 58:         var expandShrinkTests = String.join(",", SHAPE_LIST.stream()

Use `collect(joining(","))` here and in other places.

test/hotspot/jtreg/compiler/vectorapi/reshape/tests/TestVectorCast.java line 39:

> 37:  * In each cast, the VectorCastNode is expected to appear exactly once.
> 38:  */
> 39: public class TestVectorCast {

At some point i would like to explore autogenerating such files., from another Java program and a template mechanism (unfortunately we cannot use a library like Java Poet). Not now though! 

This also relates to redesigning the existing vector unit tests, moving away from a bash script to a more flexible Java program, from which we can generate unit tests and/or IR tests.

The challenge with the IR tests is knowing associated IR nodes and the set supported on various platforms. I like how you enumerated the list for the conversions. Ideally we could go to the source of truth in C2 and determine that, but it does not seem easy to determine.

test/hotspot/jtreg/compiler/vectorapi/reshape/utils/TestCastMethods.java line 42:

> 40:             makePair(BSPEC64, ISPEC128),
> 41:             makePair(BSPEC64, FSPEC128),
> 42:             // makePair(BSPEC64, DSPEC256),

May later we consider a negative test that we don't expect the required IR node(s). I am uncertain in these cases whether it's an implementation restriction or a hardware restriction, if the former when the restriction goes away the test will fail and we update it.

test/hotspot/jtreg/compiler/vectorapi/reshape/utils/VectorReshapeHelper.java line 296:

> 294:             for (int i = 0; i < osp.vectorByteSize(); i++) {
> 295:                 int expected = i < isp.vectorByteSize() ? UnsafeUtils.getByte(input, ibase, i) : 0;
> 296:                 int actual = UnsafeUtils.getByte(output, obase, i);

When `MemorySegment` previews we can remove the use of unsafe accesses, since we can view the array as a segment. Same i think also applies to direct VH accessors.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6724