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

[Marc Chevalier]

On Fri, 21 Feb 2025 18:53:28 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Remove useless local, with especially helpful name
>>  - rename
>>  - Add test suggested by @dean-long exhibiting the difference between (x << 30) << 3 and x << 33
>
> 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`.

Done. I've renamed to `collapse_nested_shift_left`, it looks more explicit to me. Feel free to tell me if you think there is better.

I was inspired by the functions above that are in camelCase, unfortunately.

> 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)) {

And the variable name doesn't bring much. Done.

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

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