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

[Marc Chevalier]

On Mon, 10 Mar 2025 15:50:47 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Random testing, trying...
>
> src/hotspot/share/opto/memnode.cpp line 3546:
> 
>> 3544: // is valid if:
>> 3545: // - conIL >= conIR
>> 3546: // - conIR <= num_rejected_bits
> 
> Is there also a restriction on `conIL`?

No, see the examples. I've added some paragraphs that may help understanding why not: the shift left will only discard higher bits, and introduce 0-lower bits. It does nothing dangerous by itself. The right shift is risky because it might pull the sign bit of (v << conIL) into the bits considered by the store, and actually changing the value.

> src/hotspot/share/opto/memnode.cpp line 3581:
> 
>> 3579: // The simplification is still fine.
>> 3580: //
>> 3581: // ### Case 3: conIL > conIR < num_rejected_bits.
> 
> Suggestion:
> 
> // ### Case 3: conIL > conIR
> 
> Or do you need that? And if so, do we only have `conIR < num_rejected_bits`, or also `conIL < num_rejected_bits`.
> A combination of `>` and `<` in the same equation can be a little confusing ;)

What I wrote is exactly what I meant. I do need `conIR < num_rejected_bits` (see former case 4, new case 2.3 for what happens otherwise). I don't need `conIL < num_rejected_bits` (see this example (former case 3, new case 2.2) where conIL = 26 > num_rejected_bits = 24).

> src/hotspot/share/opto/memnode.cpp line 3593:
> 
>> 3591: // +------------------+---------+-----+
>> 3592: //  31              10 9       4 3   0
>> 3593: // The non-rejected bits are the 8 lower one of (v << conIL - conIR).
> 
> Suggestion:
> 
> // The non-rejected bits are the 8 lower ones of (v << conIL - conIR).
> 
> But it seems we only actually kept 6 of them?

nice

> src/hotspot/share/opto/memnode.cpp line 3595:
> 
>> 3593: // The non-rejected bits are the 8 lower one of (v << conIL - conIR).
>> 3594: // The bits 6 and 7 of v have been thrown away after the shift left.
>> 3595: // The bits 4 and 5 of v are still present, but outside of the kept bits (the 8 lower ones).
> 
> I have trouble understanding this line. Bits 0-5 are still present. What do you mean by "outside of the kept bits"?

Rephrased. Might be clearer now.

> src/hotspot/share/opto/memnode.cpp line 3621:
> 
>> 3619: // Valid if conIL >= conIR <= num_rejected_bits
>> 3620: //
>> 3621: // We do not treat the case conIR > conIL here since the point of this function is
> 
> Suggestion:
> 
> // We do not treat the case conIL < conIR here since the point of this function is
> 
> Nit: just to keep the two values on the sides we have used up to now.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1988930285
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1988930385
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1988931045
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1988931886
PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1988936395