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

[Emanuel Peter]

On Tue, 27 Feb 2024 20:59:14 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 five additional commits since the last revision:
> 
>  - Merge branch 'master' into round-v-exhaustive-tests
>  - fix issue
>  - mv tests
>  - use IR framework to construct the random tests
>  - Initial commit

Thanks for changing to randomness. Thanks very much for your work!

I have a few more requests/suggestions/questions :)

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

> 30:  * @requires vm.compiler2.enabled
> 31:  * @requires (vm.cpu.features ~= ".*avx512dq.*" & os.simpleArch == "x64") |
> 32:  *           os.simpleArch == "aarch64"

We should be a able to run the tests on all platforms, with any compiler.
But you can add platform restrictions to the IR rules, with `appliyIf...`.

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

> 33:  *
> 34:  * @library /test/lib /
> 35:  * @run driver compiler.vectorization.TestRoundVectorDoubleRandom

Suggestion:

 * @run main compiler.vectorization.TestRoundVectorDoubleRandom

Driver setting apparently does not allow passing flags from the outside.

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

> 89:     test_round(res, input);
> 90:     // skip test/verify when warming up
> 91:     if (runInfo.isWarmUp()) {

Hmm. This means that if there is a OSR compilation during warmup, we would not verify. Are we ok with that?

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

> 99:     final int f_width = e_shift;
> 100:     final long f_bound = 1 << f_width;
> 101:     final int f_num = 256;

Code style: you are generally not supposed to use under_score for variables, but camelCase, I think.

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

> 109:     fis[fidx++] = 0;
> 110:     for (; fidx < f_num; fidx++) {
> 111:       fis[fidx] = ThreadLocalRandom.current().nextLong(f_bound);

Why are you not using `rand`?

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

> 122:         bits = bits | (1 << 63);
> 123:         input[ei_idx*2+1] = Double.longBitsToDouble(bits);
> 124:       }

Why do all this complicated stuff, and not just pick a random `long`, and convert it to double with `Double.longToDoubleBits`?

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

> 132:         for (int sign = 0; sign < 2; sign++) {
> 133:           int idx = ei_idx * 2 + sign;
> 134:           if (res[idx] != Math.round(input[idx])) {

Is it ok to use `Math.round` here? What if we compile it, and its computation is wrong in the compilation?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17753#pullrequestreview-1931612345
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521800236
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521801642
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521807957
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521809457
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521813203
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521815343
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1521816420