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

[Hannes Greule]

On Mon, 16 Jun 2025 06:57:16 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> @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
>
>> @eme64 I merged master and hopefully addressed your latest comments. Now that we have #17508 integrated, I could also directly update the unsigned variant, but I'm also fine with doing that separately. WDYT?
>> 
>> I also checked the constant folding part again (or generally whenever the RHS is a constant), these code paths are indeed not used by PhaseGVN directly (but by PhaseCCP and PhaseIdealLoop). That makes it a bit difficult to test that part properly.
> 
> Let's keep the patch as it is. With #17508 we will have to also probably refactor and add more tests, if we want to do any unsigned and known-bit optimizations.
> 
> ----------------
> 
> @SirYwell Thanks for the updates, I had a few more comments, but we are getting there :)

@eme64 I addressed your latest comments now, please re-review :)

Regarding my previous observation

> * If the divisor is a constant, we will directly replace the `Mod(I|L)Node` with more but less expensive nodes in `::Ideal()`. Type analysis for these nodes combined is less precise, means we miss potential cases were this would help e.g., removing range checks. Would it make sense to delay the replacement?

should I open a new RFE for that? Or generally, what's your opinion on this?

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

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