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

[Emanuel Peter]

On Mon, 17 Mar 2025 21:01:23 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:
> 
>   fix ((x<<y) << z) + (x<<y) patterns

Changes requested by epeter (Reviewer).

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

> 413:   if (find_power_of_two_addition_pattern(this, bt).valid) {
> 414:     return nullptr;
> 415:   }

Hmm. So somewhere we would have generated that pattern, probably in MulNode. Can you add a verification there, to check that we are only generating patterns that `find_power_of_two_addition_pattern` recognizes? That would make sure that we keep the code here and there in sync.

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

> 426:       ((mul = find_simple_multiplication_pattern(in1, bt)).valid && mul.variable == in2) ||
> 427:       ((mul = find_power_of_two_addition_pattern(in1, bt)).valid && mul.variable == in2)
> 428:       ) {

I find this quite difficult to read. And it looks repetitive too. Maybe you can refactor it?
Also, it would be nice to have comments with the patterns here, to see which one covers what case, so that we have a nice overview.

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

> 429:     Node* con = (bt == T_INT)
> 430:                 ? (Node*) phase->intcon((jint) (mul.multiplier + 1)) // intentional type narrowing to allow overflow at max_jint
> 431:                 : (Node*) phase->longcon((mul.multiplier + 1));

I think just to be safe, you should use `java_add` to have correct overflow semantics. You are using `jlong` for `multiplier`, which is a signed integer type, and in C++ overflow is undefined behavior as far as I know, let's avoid that ;)

Actually, do you have a test where the multiplier overflows here?

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

> 507:     if (rhs.valid && rhs.variable == n->in(1)) {
> 508:       return Multiplication{true, rhs.variable, rhs.multiplier + 1};
> 509:     }

Hmm, it seems these are patterns that you did not promise you would cover in the description above.
It makes it a little difficult to keep the overview...

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

PR Review: https://git.openjdk.org/jdk/pull/23506#pullrequestreview-2735724858
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2024508253
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2024512437
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2024530927
PR Review Comment: https://git.openjdk.org/jdk/pull/23506#discussion_r2024545411