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

[Jasmine Karthikeyan]

On Wed, 5 Feb 2025 14:11:25 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This change refactors `RShiftI`/`RshiftL` `Ideal`, `Identity` and
>> `Value` because the `int` and `long` versions are very similar and so
>> there's no logic duplication. In the process, support for some extra
>> transformations is added to `RShiftL`. I also added some new test
>> cases.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review

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.

src/hotspot/share/opto/mulnode.cpp line 1325:

> 1323:   // Check for (x & 0xFF000000) >> 24, whose mask can be made smaller.
> 1324:   // Such expressions arise normally from shift chains like (byte)(x >> 24).
> 1325:   const Node *mask = in(1);

Suggestion:

  const Node* mask = in(1);

src/hotspot/share/opto/mulnode.cpp line 1367:

> 1365: const Type* RShiftNode::ValueIL(PhaseGVN* phase, BasicType bt) const {
> 1366:   const Type *t1 = phase->type(in(1));
> 1367:   const Type *t2 = phase->type(in(2));

Suggestion:

  const Type* t1 = phase->type(in(1));
  const Type* t2 = phase->type(in(2));

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?

src/hotspot/share/opto/mulnode.cpp line 1462:

> 1460:     return progress;
> 1461:   }
> 1462:   const TypeInt *t3;  // type of in(1).in(2)

Suggestion:

  const TypeInt* t3;  // type of in(1).in(2)

src/hotspot/share/opto/mulnode.cpp line 1517:

> 1515: }
> 1516: 
> 1517: Node *RShiftLNode::Ideal(PhaseGVN *phase, bool can_reshape) {

Suggestion:

Node* RShiftLNode::Ideal(PhaseGVN *phase, bool can_reshape) {

src/hotspot/share/opto/mulnode.hpp line 322:

> 320:   virtual Node* Identity(PhaseGVN* phase);
> 321: 
> 322:   virtual Node* Ideal(PhaseGVN *phase, bool can_reshape);

Suggestion:

  virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);

src/hotspot/share/opto/type.cpp line 1533:

> 1531: }
> 1532: 
> 1533: const TypeInteger *TypeInteger::make(jlong lo, BasicType bt) {

Suggestion:

const TypeInteger* TypeInteger::make(jlong lo, BasicType bt) {

src/hotspot/share/utilities/globalDefinitions.hpp line 799:

> 797:     return BitsPerJavaInteger;
> 798:   }
> 799:   return BitsPerJavaLong;

I think it'd be nice to add `assert(bt == T_LONG, "unsupported");` before the last return, like in the helper methods above.

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

PR Review: https://git.openjdk.org/jdk/pull/23438#pullrequestreview-2596217801
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943215091
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943210959
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943233567
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943209867
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943209437
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943219249
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943207959
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1943208909