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

[Emanuel Peter]

On Fri, 13 Jun 2025 08:05:28 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

>> This change improves the precision of the `Mod(I|L)Node::Value()` functions.
>> 
>> I reordered the structure a bit. First, we handle constants, afterwards, we handle ranges. The bottom checks seem to be excessive (`Type::BOTTOM` is covered by using `isa_(int|long)()`, the local bottom is just the full range). Given we can even give reasonable bounds if only one input has any bounds, we don't want to return early.
>> The changes after that are commented. Please let me know if the explanations are good, or if you have any suggestions.
>> 
>> ### Monotonicity
>> 
>> Before, a 0 divisor resulted in `Type(Int|Long)::POS`. Initially I wanted to keep it this way, but that violates monotonicity during PhaseCCP. As an example, if we see a 0 divisor first and a 3 afterwards, we might try to go from `>=0` to `-2..2`, but the meet of these would be `>=-2` rather than `-2..2`. Using `Type(Int|Long)::ZERO` instead (zero is always in the resulting value if we cover a range).
>> 
>> ### Testing
>> 
>> I added tests for cases around the relevant bounds. I also ran tier1, tier2, and tier3 but didn't see any related failures after addressing the monotonicity problem described above (I'm having a few unrelated failures on my system currently, so separate testing would be appreciated in case I missed something).
>> 
>> Please review and let me know what you think.
>> 
>> ### Other
>> 
>> The `UMod(I|L)Node`s were adjusted to be more in line with its signed variants. This change diverges them again, but similar improvements could be made after #17508.
>> 
>> During experimenting with these changes, I stumbled upon a few things that aren't directly related to this change, but might be worth to further look into:
>> - 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?
>> - To force non-negative ranges, I'm using `char`. I noticed that method parameters of sub-int integer types all fall back to `TypeInt::INT`. This seems to be an intentional change of https://github.com/openjdk/jdk/commit/200784d505dd98444c48c9ccb7f2e4df36dcbb6a. The bug report is private, so I can't really judge if that part is necessary, but it seems odd.
>
> 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/2453dbbb...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?

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?

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

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