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

[Marc Chevalier]

On Fri, 7 Mar 2025 12:22:50 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   + comment on why not zerocon
>
> src/hotspot/share/opto/memnode.cpp line 3523:
> 
>> 3521: // (StoreB ... (valIn) )
>> 3522: // If (conIL > conIR) we are inventing 0 lower bits, and throwing
>> 3523: // away upper bits, but we are not introducing garbage bits by above.
> 
> What do you mean with `by above`?

I mean from the upper bits, from the left if written like decimal numbers. Rephrasing that.

> src/hotspot/share/opto/memnode.cpp line 3527:
> 
>> 3525: // (StoreB ... (LShiftI _ valIn (conIL - conIR)) )
>> 3526: // This case happens when the store source was itself a left shift, that gets merged
>> 3527: // into the inner left shift of the sign-extension.
> 
> Hmm, above you were talking about a left and a right shift. But now you seem to be talking about some "source" left shift and an "inner" left shift. It's a bit confusing. Are you talking about this case?
> `(StoreB ... (RShiftI _ (LShiftI _ (LShiftI _ valIn conIL1 ) conIL2 ) conIR) )`
> Where the two left shifts are already combined by Ideal earlier?

Yes indeed. You seem confused about the term "source". I'm happy to change it, but I can't see a better word: a store (or move, or assignment) has a source and a destination, that are often resp. the left-hand side and the right-hand side of the expression/statement/instruction. I'm rephrasing that.

> src/hotspot/share/opto/memnode.cpp line 3576:
> 
>> 3574:   Node* val = in(MemNode::ValueIn);
>> 3575:   if (val->Opcode() == Op_RShiftI) {
>> 3576:     const TypeInt* conIR = phase->type(val->in(2))->isa_int();
> 
> Suggestion:
> 
>   Node* shr = in(MemNode::ValueIn);
>   if (shr->Opcode() == Op_RShiftI) {
>     const TypeInt* conIR = phase->type(shr->in(2))->isa_int();
> 
> Might as well to keep it consistent with `shl` below.

Better indeed. Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1985124253
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1985124188
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1985124661