RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v6]

[Emanuel Peter]

On Thu, 14 Mar 2024 11:23:56 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> HI,
>> Can you have a look at this patch adding some tests for Math.round instrinsics?
>> Thanks!
>> 
>> ### FYI:
>> During the development of RoundVF/RoundF, we faced the issues which were only spotted by running test exhaustively against 32/64 bits range of int/long.
>> It's helpful to add these exhaustive tests in jdk for future possible usage, rather than build it everytime when needed.
>> Of course, we need to put it in `manual` mode, so it's not run when `-automatic` jtreg option is specified which I guess is the mode CI used, please correct me if I'm assume incorrectly.
>
> Hamlin Li 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 ten additional commits since the last revision:
> 
>  - Fix test failure in TestRoundVectorDoubleRandom.java
>  - Merge branch 'master' into round-v-exhaustive-tests
>  - add comments; refine code
>  - refine code; fix bug
>  - Merge branch 'master' into round-v-exhaustive-tests
>  - fix issue
>  - mv tests
>  - use IR framework to construct the random tests
>  - Initial commit

Thanks for the changes, the comments help!

You could still use full names, rather than `e` and `f`.

Left a few comments. There are a few int/long issues like this:

jshell> int x = 52;
x ==> 52

jshell> long y = 1 << x;
y ==> 1048576

jshell> long y = 1L << x
y ==> 4503599627370496

I think I understand how you want to do the generation now. But as you see, there was a bug in it. And it was hard to find. That is why I was asking for just pure random value generation. Constructing values ourselves often goes wrong, and then the test-coverage drops quickly.

Also: what is the probability that you will ever generate a `infty` or a `Nan` for doubles? Can you give me an estimate?

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 57:

> 55:   public static void main(String args[]) {
> 56:     TestFramework.runWithFlags("-XX:-TieredCompilation", "-XX:CompileThresholdScaling=0.3", "-XX:MaxVectorSize=16");
> 57:     TestFramework.runWithFlags("-XX:-TieredCompilation", "-XX:CompileThresholdScaling=0.3", "-XX:MaxVectorSize=32");

You should either drop the `"-XX:MaxVectorSize=16"`, or at least have a run without this flag.
There are machines with higher max vector length, i.e. AVX512 have `64`. Would be nice to test those too ;)

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 93:

> 91: 
> 92:     int errn = 0;
> 93:     // a double precise float point is composed of 3 parts: sign/e(exponent)/f(signicant)

Suggestion:

    // a double precise float point is composed of 3 parts: sign/e(exponent)/f(signicand)

significand != significant
;)

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 100:

> 98:     // f (significant) part of a float value
> 99:     final int fWidth = eShift;
> 100:     final long fBound = 1 << fWidth;

Suggestion:

    final long fBound = 1L << fWidth;

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 107:

> 105:     int fidx = 0;
> 106:     for (; fidx < fWidth; fidx++) {
> 107:       fis[fidx] = 1 << fidx;

Suggestion:

      fis[fidx] = 1L << fidx;

One more here, please be careful with this!

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 112:

> 110:       fis[fidx] = rand.nextLong(fBound);
> 111:     }
> 112:     fis[rand.nextInt(fidx)] = 0;

Suggestion:

    fis[rand.nextInt(fNum)] = 0;

I know it is equivalent, but a bit clearer ;)

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 122:

> 120:       final int eStart = rand.nextInt(9);
> 121:       final int eStep = (1 << 3) + rand.nextInt(3);
> 122:       for (int ei = eStart; ei < eBound; ei += eStep) {

Why modify the `eStart`, but not the `eBound`? Why not have both constant?

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 130:

> 128:         input[eiIdx*2] = Double.longBitsToDouble(bits);
> 129:         // negative values
> 130:         bits = bits | (1 << 63);

Suggestion:

        bits = bits | (1L << 63);

And yet another of these

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorDoubleRandom.java line 131:

> 129:         // negative values
> 130:         bits = bits | (1 << 63);
> 131:         input[eiIdx*2+1] = Double.longBitsToDouble(bits);

Is this some sort of luck, or why is this always in bounds?

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

PR Review: https://git.openjdk.org/jdk/pull/17753#pullrequestreview-1942898173
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528491128
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528503479
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528499484
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528504618
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528496276
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528507124
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528512615
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1528518614