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

[Hannes Greule]

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

>> src/hotspot/share/opto/divnode.cpp line 1236:
>> 
>>> 1234:     // We must be modulo'ing 2 int constants.
>>> 1235:     // Check for min_jlong % '-1', result is defined to be '0'
>>> 1236:     // We don't need to check for min_jint % '-1' as its result is defined when using jlong.
>> 
>> It seems both cases are "defined"... so it sounds a little strange when you say `... as its result is defined when using jlong.` Both are "defined", it would be nice if you said explicitly "how" they are defined.
>> 
>> But wait... how does this work. We used to do the same trick above for `min_jint` when using `Jint`, correct?
>> 
>> 
>>   // We must be modulo'ing 2 float constants.
>>   // Check for min_jint % '-1', result is defined to be '0'.
>>   if( i1->get_con() == min_jint && i2->get_con() == -1 )
>>     return TypeInt::ZERO;
>> 
>> 
>> Is this case here really handling that? It doesn't look like it.
>> Do we have tests for all these cases?
>
> Hmm, seems we have discussed this before... Maybe it is best to just keep the old behavior and do the test for `min_jint` as well if we have `T_INT`. I'd rather be safe.

I can add `min_jint` as a special case again. But I just had a different idea, as `x % -1 == 0` for any `x`, I could also generalize the check and only test for `-1`. WDYT?

>> src/hotspot/share/opto/divnode.cpp line 1244:
>> 
>>> 1242:   // The magnitude of the divisor is in range [1, 2^31] or [1, 2^63], depending on the BasicType.
>>> 1243:   // We know it isn't 0 as we handled that above.
>>> 1244:   // That means at least one value is nonzero, so its absolute value is bigger than zero.
>> 
>> I'm actually struggling to follow this here. Can you define "magnitude" for the reader? Maybe there is some JVMS definition you can mention. And which "value" are you refering to, that is nonzero here?
>
> Suggestion:
> 
>   // We checked that t2 is not the zero constant. Hence at least i2->_lo or i2->_hi must be non-zero,
>   // and hence its its absoute value is bigger than zero. Hence, the magnitude of the divisor (i.e. the
>   // largest absolute value for any value in i2) must be in the  range [1, 2^31] or [1, 2^63], depending
>   // on the BasicType.

Magnitude is what the JVMS uses, that's why I used it. But I like your suggested wording, I'll adapt it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2149519211
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2149522952