RFR: 8325438: Add exhaustive tests for Math.round intrinsics

[Hamlin Li]

On Thu, 22 Feb 2024 13:51:01 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Thanks all for your suggestions.
>> 
>> @chhagedorn @eme64 
>> I just tried to modify the test with IR test framework, but seems it requires running in driver mode, i.e. '@run driver`, and seems `driver` mode can not be run in `manual` at the same time.  Any suggestion? Or should I just follow @vnkozlov 's sugguestion to make sure golden value is retrieved via an intepreted method without compilation?
>> 
>> @chhagedorn 
>>> It's good to add some tests for that. Have you considered using IR tests instead? This could simplify the test and result verification and also add the benefit of sanity checking whether we actually used the intrinsic with matching RoundD in the IR, for example.
>> 
>> For the IR verification, there is already test cover this case, so maybe we can skip it in this test? Although it does not harm to verify IR in this test, but as above mentioned `driver` can not work together with `manual`, so we are good to just skip this case in this test?
>> 
>> @eme64
>>> It would be nice to also have different kinds of inputs: randomized, and for floats also inf, nan, etc.
>> 
>> The test is verifying the whole range of 32/64 bits, so I think it includes all special values, like nan, inf, etc, and randam is not necessary anymore in this sense. Does this make sense?
>> 
>>> I think your new tests should not go into an old "cr" directory.
>> 
>> I will move the test to other directory rather than "cr".
>> 
>>> Until that is all in place, you should do it like in this test:
>>> test/hotspot/jtreg/compiler/vectorization/TestOptionVectorizeIR.java
>>> (you can see the gold values, and the @IR rules)
>> 
>> As we can not use IR test framework, this is out of discussion?
>> But, we can make sure to use the golden value from an interpreted method as @vnkozlov suggested.
>
> @Hamlin-Li Thanks for the work and your response!
> Ah, I see now. This is a `manual` test. I did not even know that this mode existed!
> 
> A few comments:
> - We don't have any other manual tests. These tests are only run manually, and not automatically. Probably nobody will ever run this test again, hardly anybody knows that this option even exists.
> - You iterate over a range of `2^64`. How long does this take on your machine? Did this even ever complete? This will run in the order of days, if not weeks or months on a single machine.
> - You construct the float/double values manually, by adding mantissa and exponent. But what about all the values that are outside this range? Or are you sure you covered all `2^64` bit values?
> - You still compare both in potentially compiled code versions. That will not do, we may mis-compile both in the same way.
> - You don't have any IR verification about what was done by the compiler.
> 
> Suggestions:
> - Your test must be executable in automatic mode, and not just in manual. This ensures we run the test in CI, and here on GitHub actions.
> - Instead of iterating over **all possible values**, you should do it **randomly**. This way the test can run for a limited number or inputs, and terminate in reasonable time.
> - You should do it using the IR framework, so you can verify the IR nodes. The IR framework will soon support automatic random input generation and result verification. But for now you will have to do the input generation and verification against "golden" value from the interpreter yourself.
> - The random values should mix in `nan, infty, +-0.0, ...` with a higher frequency than just random values taken via `random.nextLong()` converted to double via `Double.longBitsToDouble`.
> 
> Other thoughts:
> - If your really really want the manual mode with exhaustive iteration over int and long range, then you can put that in, but only in addition to a IR framework test with random values.
> - Rather than focusing on doing just `Math.round`, we should probably do this with other (and maybe all) operations. I quickly checked for things like `Math.sqrt` and a few others. For many there is no good random input test with result verification.

Hey @eme64 , Can you have another look? Thanks!

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

PR Comment: https://git.openjdk.org/jdk/pull/17753#issuecomment-2036559409