RFR: 8347555: [REDO] C2: implement optimization for series of Add of unique value [v20]

[Roland Westrelin]

On Thu, 2 Oct 2025 22:28:29 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> [JDK-8347555](https://bugs.openjdk.org/browse/JDK-8347555) is a redo of [JDK-8325495](https://bugs.openjdk.org/browse/JDK-8325495) was [first merged](https://git.openjdk.org/jdk/pull/20754) then backed out due to a regression. This patch redos the feature and fixes the bit shift overflow problem. For more information please refer to the previous PR.
>> 
>> When constanlizing multiplications (possibly in forms on `lshifts`), the multiplier is upgraded to long and then later narrowed to int if needed. However, when a `lshift` operand is exactly `32`, overflowing an int, using long has an unexpected result. (i.e., `(1 << 32) = 1` and `(int) (1L << 32) = 0`)
>> 
>> The following was implemented to address this issue.
>> 
>> if (UseNewCode2) {
>>     *multiplier = bt == T_INT
>>         ? (jlong) (1 << con->get_int()) // loss of precision is expected for int as it overflows
>>         : ((jlong) 1) << con->get_int();
>> } else {
>>     *multiplier = ((jlong) 1 << con->get_int());
>> }
>> 
>> 
>> Two new bitshift overflow tests were added.
>
> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   refactor Multiplication into a class

Changes requested by roland (Reviewer).

src/hotspot/share/opto/addnode.cpp line 447:

> 445:   }
> 446: 
> 447:   Node* con = (bt == T_INT)

With an `if`, the `static_cast<Node*>` are not needed, right? I would do that then. It would more readable.

src/hotspot/share/opto/addnode.cpp line 494:

> 492: AddNode::Multiplication AddNode::Multiplication::find_simple_addition_pattern(const Node* n, BasicType bt) {
> 493:   if (n->Opcode() == Op_Add(bt) && n->in(1) == n->in(2)) {
> 494:     return {n->in(1), 2};

You should use a constructor call here.

src/hotspot/share/opto/addnode.cpp line 509:

> 507:     Node* con = n->in(2);
> 508:     if (!con->is_top()) {
> 509:       return {n->in(1), java_shift_left(1, con->get_int(), bt)};

You should use a constructor call here.

src/hotspot/share/opto/addnode.cpp line 524:

> 522:   if (n->Opcode() == Op_Mul(bt) && (n->in(1)->is_Con() || n->in(2)->is_Con())) {
> 523:     // Pattern (1)
> 524:     Node* con = n->in(1);

Isn't con always input 2 because `MulNode::Ideal` canonicalize it?

src/hotspot/share/opto/addnode.cpp line 534:

> 532: 
> 533:     if (!con->is_top()) {
> 534:       return {base, con->get_integer_as_long(bt)};

You should use a constructor call here.

src/hotspot/share/opto/addnode.cpp line 564:

> 562:     // Pattern (2)
> 563:     if (lhs.is_valid_with(n->in(2))) {
> 564:       return {lhs.variable(), java_add(lhs.multiplier(), static_cast<jlong>(1))};

You should use a constructor call here.

src/hotspot/share/opto/addnode.cpp line 569:

> 567:     // Pattern (3)
> 568:     if (rhs.is_valid_with(n->in(1))) {
> 569:       return {rhs.variable(), java_add(rhs.multiplier(), static_cast<jlong>(1))};

You should use a constructor call here.

src/hotspot/share/opto/addnode.hpp line 74:

> 72:     Multiplication add(const Multiplication rhs) const {
> 73:       if (is_valid_with(rhs.variable()) && rhs.is_valid_with(variable())) {
> 74:         return {variable(), java_add(multiplier(), rhs.multiplier())};

You should use a constructor call here

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

PR Review: https://git.openjdk.org/jdk/pull/23506#pullrequestreview-3303979449
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405860167
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405815427
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405816662
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405847407
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405819342
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405784710
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405785351
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2405781525