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

[Roland Westrelin]

On Wed, 1 Oct 2025 04:48:35 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 with a new target base due to a merge or a rebase. The pull request now contains 69 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into arithmetic-canonicalization
>  - update naming and comments
>  - Merge branch 'openjdk:master' into arithmetic-canonicalization
>  - Merge remote-tracking branch 'origin/master' into arithmetic-canonicalization
>  - Allow swapping LHS/RHS in case not matched
>  - Merge branch 'refs/heads/master' into arithmetic-canonicalization
>  - improve comment readability and struct helper functions
>  - remove asserts, add more documentation
>  - fix typo: lhs->rhs
>  - update comments
>  - ... and 59 more: https://git.openjdk.org/jdk/compare/0366d882...ce23d393

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

> 553: 
> 554:     // Pattern (1)
> 555:     if (lhs.valid && rhs.valid && lhs.variable == rhs.variable) {

Would it make sense to add a method to `Multiplication`, let's say `add`. The 3 `if`s here would then be replaced by:

Multiplication res = lhs.add(rhs);
if (res.valid()) {
  return res;
}
return find_simple_addition_pattern(n, bt);

and the logic from the 3 ifs would be moved to `Multiplication::add`.
What do you think?

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

> 44:   virtual uint hash() const;
> 45: 
> 46:   struct Multiplication {

Is there a benefit to this being a `struct` instead of a `class`?

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

> 45: 
> 46:   struct Multiplication {
> 47:     bool valid = false;

field names usually start with a `_` so `bool _valid`

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

> 58:   };
> 59: 
> 60:   static Multiplication find_collapsible_addition_patterns(const Node* a, const Node* pattern, BasicType bt);

Shouldn't these methods be member methods of `Multiplication`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2397605619
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2397579068
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2397581099
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2397584858