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

[Andrew Haley]

On Tue, 9 Apr 2024 15:59:20 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Does that work for you?
>
> Thanks for the sample code.
> 
> I modify the current test a bit by using your code, i.e. change from 2 level nested loop to single while loop as below, let's call it `new test`
> 
>   @Test
>   static boolean test(int testInt) {
>         float testFloat = Float.intBitsToFloat(testInt);
>         return Math.round(testFloat) != golden_round(testFloat);
>   }
> 
>   @Run(test = "test")
>   static void test_rounds(RunInfo runInfo) {
>     for (int i = 0; i < 1000; i++) {        
>         test(i);
>     }
>     if (runInfo.isWarmUp()) {
>         return;
>     }
>     boolean runTest = true; // modify here to have try.
>     if (!runTest) return;
>     int testInt = 0;
>     boolean fail = false;
>     do {
>         fail |= test(testInt);
>     } while (++testInt != 0);
>     if (fail) {
>         throw new RuntimeException();
>     }
>   }
> 
> 
> It still took more than 5 minutes to finish the test; if I assign `runTest = false`, it will take seconds. So most of time is spent on the while loop in `test_rounds` with `@Run` annotation in new test, I'm not sure how the annotation @Run works, but seems that's the reason why it's slower than a pure while loop (in your sample code). But we need the annotations in the test (check below).
> There are still some gaps between this new test and current test:
> * we still not yet verify IR Node (`IRNode.ROUND_VF`); to verify it, we need to put the (part of) test into a nested loop, and put this loop in a function (`test_round` in current test), and annotate this function with `@IR` to verify the IR node.
> 
> Or maybe there are other ways to implement this test and qualify below requirements? Currently I'm not sure.
> 1. run in a minute, as we want it to be an automatic test,
> 2. verify Math.round (intrinsic) result,
> 3. verify IR node (`IRNode.ROUND_VF) generation,
> 4. make sure all the verification is done after the warmup.

Yes, I see. The `@Run` annotation runs it all with the IR framework, which you don't really need for an exhaustive test over the 32-bit range.

I think I may have been rather misled by the "Add exhaustive tests for Math.round intrinsics" title, which this PR doesn't do. I strongly suggest that you do add an exhaustive test for the 32-bit range, without using the `@Run` annotation, just the bare code.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1559818903