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

Emanuel Peter epeter at openjdk.org
Wed Mar 13 06:59:22 UTC 2024


On Tue, 12 Mar 2024 20:16:02 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Does this ever generate things like `+0, -0, infty, NaN` etc?
>
> It's testing following cases:
> 1. all the `e` range, e.g. for double it's 11 bits, for float it's 8 bits
> 2. for `f` I add a special value `0` explicitly `fis[fidx++] = 0;`
> 3. for sign, both `+` and `-` are tested.
> 
> So, yes, it will test cases like `+/- 0, infty, NaN`.

Can you refactor or at least comment the code a little better, or use more expressive variable names?
I'd have to spend a bit of time to understand your generation method here, and if I think that it is exhaustive and covers the special cases with enough frequency.

>> This direct comparison tells me that you are not testing `NaN`s...
>
>> Is it ok to use Math.round here? What if we compile it, and its computation is wrong in the compilation?
> 
> It's a bug, will fix.
> 
>> This direct comparison tells me that you are not testing NaNs...
> 
> this comparison is between long value, for NaN Math.round(NaN) == 0. Or maybe I misunderstood your question?

Yes, you are right. I somehow thought that `Math.round` returns a float/double. But it is int/long. So exact comparison is good 😊

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

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


More information about the hotspot-compiler-dev mailing list