RFR: 8329273: C2 SuperWord: Some basic MemorySegment IR tests [v2]

[Christian Hagedorn]

On Mon, 13 May 2024 06:03:36 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I could not find any IR vectorization tests for `MemorySegment` yet.
>> 
>> I make sure to exercise different backing types:
>> - arrays
>> - buffers
>> - native memory
>> 
>> I filed a follow-up RFE, to eventually make all cases where I have "FAILS" vectorize:
>> 
>> [JDK-8331659](https://bugs.openjdk.org/browse/JDK-8331659): C2 SuperWord: investicate failed vectorization in compiler/loopopts/superword/TestMemorySegment.java
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 25 additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8329273-memory-segment-ir-tests
>  - fix tabs
>  - speed up test
>  - small cosmetic fix
>  - make things static
>  - long loop tests
>  - handle AlignVector
>  - int cases
>  - int-index case
>  - disable mixed tests
>  - ... and 15 more: https://git.openjdk.org/jdk/compare/2faa8c83...6f760dfd

Good basic tests! I have a few minor comments but otherwise, looks good.

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 36:

> 34: /*
> 35:  * @test id=byte-array
> 36:  * @bug 8310190

Should be updated to 8329273. Same for other runs

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 166:

> 164:         String providerName = System.getProperty("memorySegmentProviderNameForTestVM");
> 165:         provider = switch (providerName) {
> 166:             case "ByteArray"        -> ( () -> { return newMemorySegmentOfByteArray(); } );

You can directly use an expression lambda without return: 

case "ByteArray"        -> (() -> newMemorySegmentOfByteArray());

But I think you can go even further and directly use a method reference:
Suggestion:

            case "ByteArray"        -> (TestMemorySegmentImpl::newMemorySegmentOfByteArray);

Same for others.

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 181:

> 179:             default -> throw new RuntimeException("Test argument not recognized: " + providerName);
> 180:         };
> 181:     }

As discussed offline, this is an interesting workaround. Maybe the IR framework could be extended at some point to simplify this.

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 187:

> 185: 
> 186:     // List of gold, the results from the first run before compilation
> 187:     Map<String,Object[]> golds = new HashMap<String,Object[]>();

You can replace these with `<>`:
Suggestion:

    // List of tests
    Map<String, TestFunction> tests = new HashMap<>();

    // List of gold, the results from the first run before compilation
    Map<String, Object[]> golds = new HashMap<>();

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 199:

> 197:         tests.put("testMemorySegmentBadExitCheck", () -> {
> 198:             return testMemorySegmentBadExitCheck(copy(a));
> 199:         });

Same as above, you can replace this with an expression lambda:
Suggestion:

        tests.put("testIntLoop_longIndex_intInvar_sameAdr_byte", 
                  () -> testIntLoop_longIndex_intInvar_sameAdr_byte(copy(a), 0));

Same for others.

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 347:

> 345: 
> 346:     static MemorySegment newMemorySegmentOfMixedBuffer() {
> 347:         switch(RANDOM.nextInt(2)) {

Suggestion:

        switch (RANDOM.nextInt(2)) {

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 354:

> 352: 
> 353:     static MemorySegment newMemorySegmentOfMixed() {
> 354:         switch(RANDOM.nextInt(3)) {

Suggestion:

        switch (RANDOM.nextInt(3)) {

test/hotspot/jtreg/compiler/loopopts/superword/TestMemorySegment.java line 447:

> 445:     @IR(counts = {IRNode.LOAD_VECTOR_B, "= 0",
> 446:                   IRNode.ADD_VB,        "= 0",
> 447:                   IRNode.STORE_VECTOR,  "= 0"},

You should use `failOn` instead of `= 0`. Same for other tests.

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

PR Review: https://git.openjdk.org/jdk/pull/18535#pullrequestreview-2051802215
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597940804
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597946319
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597942075
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597947716
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597949915
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597950088
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597950155
PR Review Comment: https://git.openjdk.org/jdk/pull/18535#discussion_r1597942531