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

[Marc Chevalier]

On Mon, 10 Mar 2025 15:55:37 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 3632:
> 
>> 3630:   if (shr->Opcode() == Op_RShiftI) {
>> 3631:     const TypeInt* conIR = phase->type(shr->in(2))->isa_int();
>> 3632:     if (conIR != nullptr && conIR->is_con() && conIR->get_con() <= num_rejected_bits) {
> 
> How do we know that `conIR` (and also `conIL`) are not negative?

That is a good one. If `maskShiftAmount` was there before (which happens in the idealization of shifts, which seems to happen even in GVN, so as far as I understand, during parsing) we put the shift in bounds with:
https://github.com/openjdk/jdk/blob/cd9f1d3d921531511a7552807d099d5d3cce01a6/src/hotspot/share/opto/mulnode.cpp#L955
and then, if it changes the value (so if the shift magnitude wasn't in [0, nbits - 1]):
https://github.com/openjdk/jdk/blob/cd9f1d3d921531511a7552807d099d5d3cce01a6/src/hotspot/share/opto/mulnode.cpp#L961-L962
So I suspect the shift will have a reasonable magnitude early enough. But indeed, I'm not sure enough, and I think an additional check doesn't hurt. On the other hand, the previous version of this wasn't performing any such check, so if it's a bug, it existed already.

Bonus: according to https://docs.oracle.com/javase/specs/jls/se23/html/jls-15.html#jls-15.19, it seems that the logic of `maskShiftAmount` is correct in case of negative numbers (just bitwise masking).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1988970004