RFR: 8356813: Improve Mod(I|L)Node::Value [v6]
Emanuel Peter
epeter at openjdk.org
Mon Aug 25 13:23:01 UTC 2025
On Mon, 11 Aug 2025 07:17:40 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 18 additional commits since the last revision:
>
> - typos
> - Merge branch 'master' into improve-mod-value
> - Merge branch 'master' into improve-mod-value
> - simplify UB/cpu exception check
> - wording
> - Address more comments
> - Merge branch 'master' into improve-mod-value
> - Add randomized test
> - Use BasicType for shared implementation
> - Update ModL comment
> - ... and 8 more: https://git.openjdk.org/jdk/compare/9e98b6eb...11210414
Sorry, I was away on summer vacation and other travel. Back to reviewing now ;)
Looks really good now. I think we can almost integrate now.
One thing I'm wondering: could this be extended to `UModI/L`? That can of course be a separate RFE as well. And yet another idea: could we use the known bits? See https://github.com/openjdk/jdk/pull/17508.
src/hotspot/share/opto/divnode.cpp line 1207:
> 1205: const Type* t2 = phase->type(in2);
> 1206: if (t1 == Type::TOP) return Type::TOP;
> 1207: if (t2 == Type::TOP) return Type::TOP;
Suggestion:
if (t1 == Type::TOP) { return Type::TOP; }
if (t2 == Type::TOP) { return Type::TOP; }
If we already touch the code, we should also fix the brackets.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25254#pullrequestreview-3151393840
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2298064321
More information about the hotspot-compiler-dev
mailing list