RFR: 8356813: Improve Mod(I|L)Node::Value [v2]

[Hannes Greule]

On Wed, 28 May 2025 09:53:32 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hannes Greule has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Update ModL comment
>>  - Use TOP instead of ZERO
>>  - Apply suggested test changes
>
> @SirYwell Thanks for looking into this, that looks promising!
> 
> I have two bigger comments:
> - Could we unify the L and I code, either using C++ templating or `BasicType`? It would reduce code duplication.
> - Can we have some tests where the input ranges are random as well, and where we check the output ranges with some comparisons?
> 
> ------------------
> Copied from the code comment:
> 
>> Nice work with the examples you already have, and randomizing some of it!
>> 
>> I would like to see one more generalized test.
>> - compute `res = lhs % rhs`
>> - Truncate both `lhs` and `rhs` with randomly produced bounds from Generators, like this: `lhs = Math.max(lo, Math.min(hi, lhs))`.
>> - Below, add all sorts of comparisons with random constants, like this: `if (res < CON) { sum += 1; }`. If the output range is wrong, this could wrongly constant fold, and allow us to catch that.
>> 
>> Then fuzz the generated method a few times with random inputs for `lhs` and `rhs`, and check that the `sum` and `res` value are the same for compiled and interpreted code.
>> 
>> I hope that makes sense :)
>> This is currently my best method to check if ranges are correct, and I think it is quite important because often tests are only written with constants in mind, but less so with ranges, and then we mess up the ranges because it is just too tricky.
>> 
>> This is an example, where I asked someone to try this out as well:
>> https://github.com/openjdk/jdk/pull/23089/files#diff-12bebea175a260a6ab62c22a3681ccae0c3d9027900d2fdbd8c5e856ae7d1123R404-R422

Thanks @eme64. I unified the code now using `BasicType`. This works well because we can use the jlong operations everywhere (if I didn't miss something, please verify that claim). You can probably compare it to the unsigned_mod_value that is currently templated. I assume using BasicType there would be more involved because signed -> unsigned conversion depends on the actual type (i.e. the unsigned value of  -1 is different for long vs int).

I'll also look into your suggestions for the tests, thanks for the input there.

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

PR Comment: https://git.openjdk.org/jdk/pull/25254#issuecomment-2918524098