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

[Emanuel Peter]

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

>> Hannes Greule has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision:
>> 
>>  - Address more comments
>>  - Merge branch 'master' into improve-mod-value
>>  - Add randomized test
>>  - Use BasicType for shared implementation
>>  - Update ModL comment
>>  - Use TOP instead of ZERO
>>  - Apply suggested test changes
>>  - adapt uabs -> g_uabs name change
>>  - change range of mod by 0 for PhaseCCP
>>  - Improve ModLNode::Value
>>  - ... and 3 more: https://git.openjdk.org/jdk/compare/f8ff7d46...77134c1a
>
> 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.

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

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