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

Hamlin Li mli at openjdk.org
Thu Feb 22 12:38:54 UTC 2024


On Wed, 7 Feb 2024 16:07:02 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.

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.

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

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


More information about the hotspot-compiler-dev mailing list