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

Hannes Greule hgreule at openjdk.org
Thu May 15 16:55:38 UTC 2025


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.

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

Commit messages:
 - adapt uabs -> g_uabs name change
 - change range of mod by 0 for PhaseCCP
 - Improve ModLNode::Value
 - ModLNode::Value tests
 - improve ModINode::Value
 - ModINode::Value tests

Changes: https://git.openjdk.org/jdk/pull/25254/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25254&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8356813
  Stats: 514 lines in 3 files changed: 477 ins; 23 del; 14 mod
  Patch: https://git.openjdk.org/jdk/pull/25254.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25254/head:pull/25254

PR: https://git.openjdk.org/jdk/pull/25254


More information about the hotspot-compiler-dev mailing list