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

[Emanuel Peter]

On Mon, 29 Apr 2024 11:38:27 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 incrementally with one additional commit since the last revision:
> 
>   fix issues; modify vm options to make sure test the expected behaviors.

Thanks for the extra tests!

Can you measure how much time each test now takes on your machine?

I think we are getting there. Still a little worried about some random bugs in the whole number generation... But I'd prefer having these tests to not having them for sure ;)

test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 31:

> 29:  * @library /test/lib /
> 30:  * @modules java.base/jdk.internal.math
> 31:  * @run main/othervm -XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 -XX:+PrintIdeal -XX:CompileCommand=compileonly,compiler.floatingpoint.TestRoundFloatAll::test* -XX:-UseSuperWord compiler.floatingpoint.TestRoundFloatAll

please break up the line for easier reading

test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 75:

> 73:         return (int) a;
> 74:     }
> 75:   }

At first, I was worried about the indentation, then realized the original code had the strange indentation.
Would there be a way to put this method in a shared file, so that you do not need to paste it everywhere?

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatAll.java line 34:

> 32:  * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=8 -XX:+UseSuperWord -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test* compiler.vectorization.TestRoundVectorFloatAll
> 33:  * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=16 -XX:+UseSuperWord -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test* compiler.vectorization.TestRoundVectorFloatAll
> 34:  * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=32 -XX:+UseSuperWord -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test* compiler.vectorization.TestRoundVectorFloatAll

Please check which flags you actually need here....

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatAll.java line 43:

> 41: public class TestRoundVectorFloatAll {
> 42:   private static final int ITERS  = 11000;
> 43:   private static final int ARRLEN = 997;

Could you randomize this value ever so slightly? That way, the boundaries of the array are at different places. I think also that the size should be a little larger, just to ensure that we get maximum vector lengths.

test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatRandom.java line 202:

> 200:     }
> 201: 
> 202:     // test cases for NaN, Inf, subnormal, and so on

just for completeness: +0.0 and -0.0

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

PR Review: https://git.openjdk.org/jdk/pull/17753#pullrequestreview-2043182218
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592477207
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592487797
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592499343
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592508616
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592481581