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

[Hannes Greule]

On Sun, 19 Oct 2025 10:01:04 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.
>
> Tobias Hotz has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Adjust long constant folding test as well
>  - Adjust test, assert and comments

I've got a few comments

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

> 649:   if( (t1 == bot) || (t2 == bot) ||
> 650:       (t1 == Type::BOTTOM) || (t2 == Type::BOTTOM) )
> 651:     return bot;

I think this can be removed - and in cases where one side is the local bottom (i.e., `TypeInt::INT`) and the other is more restricted, the result should even more precise after removing. Could you also add tests for such cases? For example dividing `TypeInt::INT` by some interval with a lower bound of 2, the resulting range can be narrowed. Similarly, dividing some small interval `[lo, hi]` by `TypeInt::INT` should result in a similar interval with bounds adjusted to deal with sign changes. If I didn't miss something, your code should already be able to deal with this, it's just this early return here preventing it.

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

> 655:   const TypeInt *i1 = t1->is_int();
> 656:   const TypeInt *i2 = t2->is_int();
> 657:   int widen = MAX2(i1->_widen, i2->_widen);

This line can be moved into `compute_signed_div_type` as well. I guess it would also be okay to adjust the formatting in the rest of the method, i.e., `T *v` -> `T* v` and `if( a )` -> `if (a)` etc.

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

> 660:     if (d == 0) {
> 661:       // this division will always throw an exception
> 662:       return Type::TOP;

I'd personally prefer to have that code directly below the other cases returning TOP. You can also probably simplify it by testing for `i2 == TypeInt::ZERO` instead.

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

> 53:         // All constants available during parsing
> 54:         return getIntConstant(50) / getIntConstant(25);
> 55:     }

Could you also add a test with randomly generated constants? Then you also don't need to use `getIntConstant` here. It would also make sense to write a short comment for `getIntConstant` to clarify why it's needed.

When dividing by a constant you might also want to make sure that other nodes generated in `transform_int/long_divide` also aren't present here anymore. I'd also assume that e.g., `v / C > Integer.MAX_VALUE / C` for a positive constant C wouldn't be fully optimized away due to https://bugs.openjdk.org/browse/JDK-8366815, can you confirm that?

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

PR Review: https://git.openjdk.org/jdk/pull/26143#pullrequestreview-3354507481
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443289805
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443296455
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443295564
PR Review Comment: https://git.openjdk.org/jdk/pull/26143#discussion_r2443302805