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

[Jasmine Karthikeyan]

On Fri, 21 Feb 2025 15:57:30 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

This is a nice improvement! I'm glad to see that the limitation of Store/Shift folding requiring both shifts to have the same constant is being fixed. I've left some comments here.

I also noticed that in `LShiftINode::Ideal`, there is a transform that mentions not breaking `i2s` and `i2b` patterns. It might be worth looking separately to see if this condition can be relaxed because of the change to `StoreNode::Ideal_sign_extended_input`.

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

> 3572: // StoreB ... (RShiftI _ (LShiftI _ (LShiftI _ valIn (conIL - conIR)) conIR ) conIR)
> 3573: Node* StoreNode::Ideal_sign_extended_input(PhaseGVN* phase, int num_rejected_bits) {
> 3574:   Node *val = in(MemNode::ValueIn);

It might be good to clean up the other 4 lines in this function to match current style guidelines while you're updating it.

src/hotspot/share/opto/mulnode.cpp line 981:

> 979: // con0 is assumed to be masked already (as computed by maskShiftAmount) and non-zero
> 980: // bt must be T_LONG or T_INT.
> 981: static Node* collapseDoubleShiftLeft(PhaseGVN* phase, Node* outer_shift, int con0, BasicType bt) {

>From the style guide, functions and local variables are named with `snake_case`. Maybe it could be named `collapse_left_shifts`.

src/hotspot/share/opto/mulnode.cpp line 986:

> 984:   Node* inner_shift = outer_shift->in(1);
> 985:   int inner_shift_op = inner_shift->Opcode();
> 986:   if (inner_shift_op != Op_LShift(bt)) {

Since the local variable is otherwise unused, it'd be simpler to do:
Suggestion:

  if (inner_shift->Opcode() != Op_LShift(bt)) {

src/hotspot/share/opto/mulnode.cpp line 996:

> 994: 
> 995:   if (con0 + con1 >= nbits) {
> 996:     return ConNode::make(TypeInteger::zero(bt));

It'd be clearer to do this, which is more equivalent but more concise:
Suggestion:

    return phase->zerocon(bt);

src/hotspot/share/opto/mulnode.cpp line 1018:

> 1016: // constant, flatten the tree: (X+con1)<<con0 ==> X<<con0 + con1<<con0
> 1017: //
> 1018: // (X << con1) << con2 ==> X << (con1 + con2) (see collapseDoubleShiftLeft for details)

I think it would be better to move this comment where `collapseDoubleShiftLeft` is called in the Ideal function, and same for `LShiftLNode::Ideal`.

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

PR Review: https://git.openjdk.org/jdk/pull/23728#pullrequestreview-2633935484
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1966017369
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1966021220
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1966018379
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1966004662
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1966006840