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

[Emanuel Peter]

On Fri, 30 May 2025 07:26:13 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 incrementally with one additional commit since the last revision:
> 
>   Add randomized test

src/hotspot/share/opto/divnode.cpp line 1206:

> 1204: 
> 1205: //------------------------------Value------------------------------------------
> 1206: static const Type* mod_value(const PhaseGVN* phase, const Node* in1, const Node* in2, const BasicType bt, const Type* bottom) {

You did choose the `bt` path here!
I would add an assert that we only allow `T_INT` and `T_LONG`

src/hotspot/share/opto/divnode.cpp line 1237:

> 1235:     // We don't need to check for min_jint % '-1' as its result is defined when using jlong.
> 1236:     if (i1->get_con_as_long(bt) == min_jlong && i2->get_con_as_long(bt) == -1) {
> 1237:       return TypeInteger::zero(bt);

Is this correct? For `bt = T_INT` is this really equivalent?

`i1->get_con() == min_jint`
We might get `min_jint` back here.

`i1->get_con_as_long(bt) == min_jlong`
Would we not return `min_jint` here, and then the condition is false?

Do we have an IR test for this?

src/hotspot/share/opto/divnode.cpp line 1241:

> 1239:     return TypeInteger::make(i1->get_con_as_long(bt) % i2->get_con_as_long(bt), bt);
> 1240:   }
> 1241:   // The magnitude of the divisor is in range [1, 2^63].

You should probably also mention the `2^31` variant.

src/hotspot/share/opto/divnode.cpp line 1247:

> 1245:   // JVMS lrem bytecode: "the magnitude of the result is always less than the magnitude of the divisor"
> 1246:   // "less than" means we can subtract 1 to get an inclusive upper bound in [0, 2^63-1]
> 1247:   jlong hi = static_cast<jlong>(divisor_magnitude - 1);

Hmm, this also looks confusing for the `T_INT` case. What about `-5`, does that then not become `max_julong - 5`, but it should have been `max_juint - 1`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2120802575
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2120800945
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2120801900
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2120805584