On Mon, 29 Apr 2024 11:38:27 GMT, Hamlin Li <m...@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