RFR: 8349361: C2: RShiftL should support all applicable transformations that RShiftI does [v2]

[Roland Westrelin]

On Wed, 5 Feb 2025 16:00:08 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

> This is really nice! I'd wondered why there was no `RShiftL::Ideal`, and it's nice to have it handled it in a generic way with the integer version. I left mostly code style comments here.

Thanks for reviewing this. I pushed a new commit that takes your comments into account.

> src/hotspot/share/opto/mulnode.cpp line 1399:
> 
>> 1397:     assert(lo <= hi, "must have valid bounds");
>> 1398: #ifdef ASSERT
>> 1399:    if (bt ==T_INT) {
> 
> Suggestion:
> 
>    if (bt == T_INT) {
> 
> Could this assert be generic to also handle T_LONG too?

The assert checks that, for the int case:


long lo;
assert((int)(lo >> shift) == (((int)lo) >> shift, "");

For long, it would be:

long lo;
assert((long)(lo >> shift) == (((long)lo) >> shift, "");

Given everything is already a long, that's:

long lo;
assert(lo >> shift == lo >> shift, "");

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

PR Comment: https://git.openjdk.org/jdk/pull/23438#issuecomment-2642478546
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1946277125