RFR: 8364766: Improve Value() of DivI and DivL for non-constant inputs

Thu Aug 7 11:08:19 UTC 2025

On Sun, 6 Jul 2025 08:08:25 GMT, Tobias Hotz <duke at openjdk.org> wrote:

> This PR improves the value of interger division nodes.
> Currently, we only emit a good type if either input is constant. But we can also cover the generic case. It does that by finding the four corners of the division. This is guranteed to find the extrema that we can use for min/max. Some special logic is required for MIN_INT / -1, though, as this is a special case
> We also need some special logic to handle ranges that cross zero, but in this case, we just need to check for the negative and positive range once.
> This also cleans up and unifies the code paths for DivINode and DivLNode.
> I've added some tests to validate the optimization. Without the changes, some of these tests fail.

Thank you for contributing this enhancement, @ichttt! Your code is very well commented and thus easy to follow, and you tested your changes thoroughly. Nice work!

I mainly nitpicked on a few details below. But I have one question: You went through great pains to only calculate the necessary "corners". Would it not be much easier to calculate all four possible corners and let the min and max functions deal with the duplicates in case the `i1` or `i2` range is a singleton? The result should be the same (if I did not forget about a corner case) and it would be easier to follow.

What kind of testing did you run on your side? I kicked off a tier1 through 5 and will keep you posted on the results.

You still have a title mismatch between the issue and the PR (the PR is missing "C2:").

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

> 505: }
> 506: 
> 507: 

Suggestion:

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

> 537:   // We compute all four and take the min and max.
> 538:   // A special case handles overflow when dividing the most‐negative value by −1.
> 539: 

Suggestion:

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

> 546:   assert(min_val == min_jint || min_val == min_jlong, "min has to be either min_jint or min_jlong");
> 547: 
> 548:   // Special overflow case: min_val / (-1) == min_val

Suggestion:

  // Special overflow case: min_val / (-1) == min_val (cf. JVMS§6.5 idiv/ldiv)

Since it is explicitly mentioned in the [spec](https://docs.oracle.com/javase/specs/jvms/se24/html/jvms-6.html#jvms-6.5.idiv), you may want to add a reference to it.

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

> 549:   // We must include min_val in the output if i1->_lo == min_val and i2->_hi.
> 550:   if (i1->_lo == min_val && i2_hi == -1) {
> 551:     // special case: min_jint or min_jlong div -1 == min_val

Suggestion:

With the comments above the `if`, this is superfluous.

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

> 552:     new_lo = i1->_lo;
> 553:     if (!i1->is_con()) {
> 554:       // Also compute the “next” division result for a non‑constant range.

Suggestion:

      // Also compute the "next" division result for a non‑constant range.

Nit: let's stick to ASCII for the quotes :)

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

> 566:   if (i2_lo != i2_hi) {
> 567:     // special case not possible here, _lo mus
> 568:     assert(i2_lo != -1, "Special case not possible here");

While functionally correct, here you are only talking about the negative special case, but if `i2_lo in [0,1]` the same might happen on the positive side.
Suggestion:

  // If the divisor range is wider than a singleton, include (i1->_lo, i2->_lo).
  // We cannot use is_con here, as a range of [-1, 0] for i2_hi and [0,1] for i2_lo
  // will also result in i2_lo and i2_hi being -1, or i2_lo and i2_hi being 1
  // respectively.
  if (i2_lo != i2_hi) {
    assert(i2_hi - i2_lo >= 1, "i2 must be wider that a singleton");

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

> 573: 
> 574:   // If i1 is not a single constant, include the two corners with i1->_hi:
> 575:   //   (i1->_hi, i2->_lo) and (i1->_hi, i2->_hi)

Why do you not have to handle the case of `i2` being a singleton range?

test/hotspot/jtreg/compiler/c2/irTests/IntegerDivValueTests.java line 56:

> 54:     public int testIntConstantFoldingSpecialCase() {
> 55:         // All constants available during parsing
> 56:         return Integer.MIN_VALUE / -1;

It would be good to check the result of this computation, since it is a special case.

test/hotspot/jtreg/compiler/c2/irTests/IntegerDivValueTests.java line 125:

> 123:     @Run(test = {"testIntRange", "testIntRange2", "testIntRange3", "testIntRange4", "testIntRange5", "testIntRange6", "testIntRange7", "testIntRange8"})
> 124:     public void checkIntRanges(RunInfo info) {
> 125: 

Suggestion:

test/hotspot/jtreg/compiler/c2/irTests/IntegerDivValueTests.java line 173:

> 171: 
> 172:     // Long variants
> 173: 

Suggestion:

    // Long variants

test/hotspot/jtreg/compiler/c2/irTests/IntegerDivValueTests.java line 254:

> 252:     @Run(test = {"testLongRange", "testLongRange2", "testLongRange3", "testLongRange4", "testLongRange5", "testLongRange6", "testLongRange7", "testLongRange8"})
> 253:     public void checkLongRanges(RunInfo info) {
> 254: 

Suggestion:

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/26143#pullrequestreview-3095836322
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259419349
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259563833
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259601794
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259609385
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259614381
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259692276
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259879642
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259891388
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259902412
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259906531
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2259905197