RFR: 8347459: C2: missing transformation for chain of shifts/multiplications by constants [v15]

[Emanuel Peter]

On Tue, 11 Mar 2025 10:58:14 GMT, Marc Chevalier <duke at openjdk.org> wrote:

>> This collapses double shift lefts by constants in a single constant: (x << con1) << con2 => x << (con1 + con2). Care must be taken in the case con1 + con2 is bigger than the number of bits in the integer type. In this case, we must simplify to 0.
>> 
>> Moreover, the simplification logic of the sign extension trick had to be improved. For instance, we use `(x << 16) >> 16` to convert a 32 bits into a 16 bits integer, with sign extension. When storing this into a 16-bit field, this can be simplified into simple `x`. But in the case where `x` is itself a left-shift expression, say `y << 3`, this PR makes the IR looks like `(y << 19) >> 16` instead of the old `((y << 3) << 16) >> 16`. The former logic didn't handle the case where the left and the right shift have different magnitude. In this PR, I generalize this simplification to cases where the left shift has a larger magnitude than the right shift. This improvement was needed not to miss vectorization opportunities: without the simplification, we have a left shift and a right shift instead of a single left shift, which confuses the type inference.
>> 
>> This also works for multiplications by powers of 2 since they are already translated into shifts.
>> 
>> Thanks,
>> Marc
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   more checks

src/hotspot/share/opto/memnode.cpp line 3520:

> 3518: // Check for useless sign-extension before a partial-word store
> 3519: // (StoreB ... (RShiftI _ (LShiftI _ v conIL) conIR))
> 3520: // If (conIL == conIR && conIR <= num_bits) this simplifies to

Suggestion:

// If (conIL == conIR && conIR <= num_rejected_bits) this simplifies to

Since we renamed the argument ;)

src/hotspot/share/opto/memnode.cpp line 3549:

> 3547: // - conIL >= conIR
> 3548: // - num_rejected_bits >= conIR
> 3549: // Remembering that only the 8 lower bits have to be correct.

To me, this is the core of the comments here, the Statement that we want to prove. so we could highlight it. Maybe like this?

Suggestion:

//
// Statement (proved further below in case analysis):
//   Given:
//   - 0 <= conIL < BitsPerJavaInteger   (no wrap in shift)
//   - 0 <= conIR < BitsPerJavaInteger   (no wrap in shift)
//   - conIL >= conIR
//   - num_rejected_bits >= conIR
//   Then this form:
//      (RShiftI _ (LShiftI _ v conIL) conIR)
//   can be replaced with this form:
//      (LShiftI _ v (conIL-conIR))
//
// Note: We only have to show that the non-rejected lowest bits (8 bits for byte) have to be correct,
//       as the higher bits are rejected / truncated by the store.

src/hotspot/share/opto/memnode.cpp line 3563:

> 3561: // ###### Case 1.1: conIL == conIR == num_rejected_bits
> 3562: // If we do the shift left then right by 24 bits, we get:
> 3563: // after << 24

Suggestion:

// after "<< 24"

This could help visually, I had to stare at it for 3 sec until I knew it was not a shifting of `after`, i.e. `after << 24`

src/hotspot/share/opto/memnode.cpp line 3606:

> 3604: // The non-rejected bits are the 8 lower bits of v. The bits 8 and 9 of v are still
> 3605: // present in (v << 22) >> 22 but will be dropped by the store. The simplification is
> 3606: // still correct.

I think you copied this from case `1.2` 😅 
This case is NOT ok to simplify, that's why we needed that condition in the "Statement", right?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r2005548215
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r2005568763
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r2005573848
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r2005585291