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

Mon May 19 09:27:56 UTC 2025

On Thu, 15 May 2025 15:13:18 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.

Thank you for working on this, @SirYwell. I especially like the citations directly from the spec to motivate and justify the optimizations.

I commented only on the `int` side of things, but the comments apply equally to the `long` changes.

You exclude zero from the dividend magnitude only based on the constant check. That is not correct. You have to check the range as well to exclude zero. Hence, it would also be nice to have test cases where the value is known to be in a given range in the ideal graph. To get such a value, you can call `array.length()`, which is always `>=0`, or use `Parse::sharpen_type_after_if()`:

https://github.com/openjdk/jdk/blob/effe40a24c29dc507eea5efef7b0736a33bc34a7/src/hotspot/share/opto/parse2.cpp#L1772-L1794

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

> 1227:   // Mod by zero?  Throw exception at runtime!
> 1228:   if (i2->is_con() && i2->get_con() == 0) {
> 1229:     return TypeInt::ZERO;

Like @merykitty , I am unsure of returning zero in this case. The original code probably returned TypeInt::POS for the same reason you bring up below:

> JVMS `irem` bytecode: "the result of the remainder operation can be negative only if the dividend is negative and can be positive only if the dividend is positive"

Hence, I would argue to keep that oldbehaviorr, since the result of a modulo with zero is not defined to be zero.

I like the idea of returning TOP, but that needs to be tested really well, since all uses of the modulo computation will get removed. I am not familiar enough with the type lattice to reason about the formal correctness of this.

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

> 1240:   // The magnitude of the divisor is in range [1, 2^31].
> 1241:   // We know it isn't 0 as we handled that above.
> 1242:   // That means at least one value is nonzero, so its absolute value is bigger than zero.

Is that really what you checked above? AFAIU, above you check whether the divisor is a zero constant. But if the divisor is not a constant, then its range might still contain zero. You should check this claim using the bounds, otherwise this will not hold.

test/hotspot/jtreg/compiler/c2/gvn/ModINodeValueTests.java line 39:

> 37:  * @bug 8356813
> 38:  * @summary Test that Value method of ModINode is working as expected.
> 39:  * @library /test/lib /

Suggestion:

 * @summary Test that Value method of ModINode is working as expected.
 * @key randomness
 * @library /test/lib /

test/hotspot/jtreg/compiler/c2/gvn/ModINodeValueTests.java line 43:

> 41:  */
> 42: public class ModINodeValueTests {
> 43:     private static final Generator<Integer> G = Generators.G.ints();

Nit: please give `G` a more meaningful name like `IntGenerator`. That way, it is easier to see what falls out of the `next()` method.

test/hotspot/jtreg/compiler/c2/gvn/ModINodeValueTests.java line 91:

> 89:     // i.e., posVal % x < 0 => false.
> 90:     public boolean nonNegativeDividend(int x) {
> 91:         return x != 0 && 123 % x < 0;

It would be nice to have random dividends in the tests as well to further increase coverage. To still get constant folding, you can put random values in final fields.

test/hotspot/jtreg/compiler/c2/gvn/ModLNodeValueTests.java line 39:

> 37:  * @bug 8356813
> 38:  * @summary Test that Value method of ModLNode is working as expected.
> 39:  * @library /test/lib /

Suggestion:

 * @summary Test that Value method of ModLNode is working as expected.
 * @key randomness
 * @library /test/lib /

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

Changes requested by mhaessig (Author).

PR Review: https://git.openjdk.org/jdk/pull/25254#pullrequestreview-2849962885
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2095215903
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2095201222
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2095218234
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2095220887
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2095224553
PR Review Comment: https://git.openjdk.org/jdk/pull/25254#discussion_r2095225118