RFR: 8373999: C2: apply KnownBits and unsigned bounds to Add / Sub operations [v4]

[Benoît Maillard]

On Sun, 28 Dec 2025 07:33:45 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> This PR improves the implementation of `AddNode/SubNode::Value` by taking advantage of the additional information in `TypeInt`. The implementation has some pretty non-trivial logic. Fortunately, the test infrastructure is already there.
>> 
>> Please take a look and leave your reviews, thanks a lot.
>
> Quan Anh Mai 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 five additional commits since the last revision:
> 
>  - copyright year
>  - Merge branch 'master' into addsub
>  - Merge branch 'master' into addsub
>  - include order
>  - Improve Add/SubNode::Value with unsigned bounds and known bits

Great work! I went through all the calculations, and tried to reproduce them independently. It all looks sound to me. I only have a few comments, mostly about notation.

src/hotspot/share/opto/rangeinference.hpp line 419:

> 417:         // Similarly, if st1._lo < 0 and st2._lo < 0, we have:
> 418:         // - 2^(n-1) <= st1._ulo <= v1 <= st1._uhi <= 2^n - 1
> 419:         // - 2^(n-1) <= st2._ulo <= v2 <= st2._uhi <= 2^n - 1

At first I thought the `-` character was "minus", which confused me a little. Maybe we should avoid this, and only have indentation?

src/hotspot/share/opto/rangeinference.hpp line 437:

> 435:         // non-negative, the signed addition does not overflow, we can compute it directly.
> 436:         lo = S<CTP>(st1._ulo + st2._ulo);
> 437:         hi = S<CTP>(st1._uhi + st2._uhi);

Why not use the signed bounds directly since they are equal anyway? I find it a bit easier to read, and we can avoid the cast.
Suggestion:

        lo = st1._lo + st2._lo;
        hi = st1._hi + st2._hi;

src/hotspot/share/opto/rangeinference.hpp line 453:

> 451:       //   sum[i] = bit & 1;
> 452:       //   carry[i - 1] = (bit >= 2);
> 453:       // }

Is there a specific reason why the notation here goes from `n-1` to `0` and not the reverse? I find it more intuitive to have index `0` for the least significant bit, but maybe there is some convention I am not aware of. It does not matter too much in any case, so feel free to do whatever.

src/hotspot/share/opto/rangeinference.hpp line 480:

> 478:       //
> 479:       // If we gather the min_bits into a value tmp, it is clear that
> 480:       // tmp = st1._bits._ones + st2._bits._ones:

It feels like we don't need to "initialize" `tmp`, but maybe I am missing something

src/hotspot/share/opto/rangeinference.hpp line 491:

> 489:       // min_bit >= 2 if and only if either:
> 490:       // - st1._bits._ones[i] == st2._bits._ones[i] == 1
> 491:       // - (st1._bits._ones[i] == 1 || st2._bits._ones[i] == 1) && ((min_bit & 1) == 0)

If I am not mistaken we could also write it this way, and I personally find this a bit more intuitive (and also more consistent with the subtraction case). And for the subsequent computations we could replace `|` by `^`. 
Suggestion:

      // - (st1._bits._ones[i] != st2._bits._ones[i]) && ((min_bit & 1) == 0)

src/hotspot/share/opto/rangeinference.hpp line 535:

> 533:         // compute the signed bounds.
> 534:         lo = S<CTP>(st1._ulo - st2._uhi);
> 535:         hi = S<CTP>(st1._uhi - st2._ulo);

Same comment as for `infer_add`
Suggestion:

        lo = st1._lo - st2._hi;
        hi = st1._hi - st2._lo;

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

PR Review: https://git.openjdk.org/jdk/pull/28897#pullrequestreview-3659450715
PR Review Comment: https://git.openjdk.org/jdk/pull/28897#discussion_r2689670444
PR Review Comment: https://git.openjdk.org/jdk/pull/28897#discussion_r2690270626
PR Review Comment: https://git.openjdk.org/jdk/pull/28897#discussion_r2689734122
PR Review Comment: https://git.openjdk.org/jdk/pull/28897#discussion_r2690053424
PR Review Comment: https://git.openjdk.org/jdk/pull/28897#discussion_r2689804049
PR Review Comment: https://git.openjdk.org/jdk/pull/28897#discussion_r2690277995