RFR: 8291809: Convert compiler/c2/cr7200264/TestSSE2IntVect.java to IR verification test [v2]

[Emanuel Peter]

On Wed, 17 Jan 2024 10:03:06 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

>> This changeset translates the tests in `compiler/c2/cr7200264/` to use the IR verification framework.
>> 
>> The proposed translation, to the extent possible, attempts to preserve the semantics of the original test. A major difference is that the IR checks are now local (for every `test_*` method) instead of global. The execution time of the new test is comparable to the old test.
>> 
>> Testing:
>> - [GitHub Actions](https://github.com/dlunde/jdk/actions/runs/7553846710)
>> - Ran the new translated tests within all tier1 through tier10 contexts on windows-x64, linux-x64, linux-aarch64, macosx-x64, and macosx-aarch64.
>> - Tested that manually adding `-XX:LoopUnrollLimit=0` to the test framework flags caused the translated tests to fail. Note: it is, however, no longer possible to break the test by passing `-XX:LoopUnrollLimit=0` on the command line.
>
> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Refactor test to use multiple @Test

A few first comments, still looking into the shift cases...

Ah, a general comment: we also care about `aarch64`, i,e, `asimd`. Not just about `sse2` ;)
Would it be reasonable to add `asimd` as well?

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 37:

> 35: 
> 36: /*
> 37:  * Based on test/compiler/6340864/TestIntVect.java without performance tests.

Suggestion:

 * Based on test/hotspot/jtreg/compiler/c2/cr6340864/TestIntVect.java without performance tests.

New path, must have been moved at some point.

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 50:

> 48:     public static void main(String[] args) {
> 49:         TestFramework.runWithFlags("-XX:+IgnoreUnrecognizedVMOptions",
> 50:                                    "-XX:StressLongCountedLoop=0");

What is this for? Maybe add a comment.

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 63:

> 61:          mode = RunMode.STANDALONE)
> 62:     public void run() {
> 63: 

Suggestion:

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 406:

> 404:         if (errn > 0) {
> 405:             System.err.println("FAILED: " + errn + " errors");
> 406:             System.exit(97);

Why not just throw an exception, with that error message?

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 412:

> 410:     }
> 411: 
> 412:     int test_sum(int[] a1) {

Suggestion:

    // Not vectorized: simple addition not profitalbe, see JDK-8307516.
    int test_sum(int[] a1) {

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 510:

> 508:     }
> 509: 
> 510:     void test_divc(int[] a0, int[] a1) {

Suggestion:

    // Not vectorized: no vector div. Might vectorize after JDK-8282365 (transform div to mul/add/shift).
    void test_divc(int[] a0, int[] a1) {

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 516:

> 514:     }
> 515: 
> 516:     void test_divc_n(int[] a0, int[] a1) {

Suggestion:

    // Not vectorized: no vector div. Might vectorize after JDK-8282365 (transform div to mul/add/shift).
    void test_divc_n(int[] a0, int[] a1) {

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 522:

> 520:     }
> 521: 
> 522:     void test_divv(int[] a0, int[] a1, int b) {

Suggestion:

    // Not vectorized: no vector div.
    void test_divv(int[] a0, int[] a1, int b) {

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 528:

> 526:     }
> 527: 
> 528:     void test_diva(int[] a0, int[] a1, int[] a2) {

Suggestion:

    // Not vectorized: no vector div.
    void test_diva(int[] a0, int[] a1, int[] a2) {

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 643:

> 641:             a0[i] = (int)(a1[i]<<(-SHIFT));
> 642:         }
> 643:     }

Not sure why these don't vectorize. Need to investigate.

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 682:

> 680:             a0[i] = (int)(a1[i]>>>(-SHIFT));
> 681:         }
> 682:     }

same here, not sure, but shift by 32bit is not great.

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 721:

> 719:             a0[i] = (int)(a1[i]>>(-SHIFT));
> 720:         }
> 721:     }

yet another shift case

test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 732:

> 730:     }
> 731: 
> 732:     void test_pack2(long[] p2, int[] a1) {

Intersting pattern! But that would require optimizations we currently do not have.
Maybe we can do that in the future, with heavy upgrades to the autovectorizer,
or other optimizations that merge loads / stores.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17428#pullrequestreview-1829487943
PR Comment: https://git.openjdk.org/jdk/pull/17428#issuecomment-1898364629
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457321852
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457325501
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457326179
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457329945
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457331858
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457335029
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457336037
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457336434
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457336611
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457338711
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457340003
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457340606
PR Review Comment: https://git.openjdk.org/jdk/pull/17428#discussion_r1457344577