RFR: 8369167: C2: refactor LShiftINode/LShiftLNode Value/Identity/Ideal [v4]

[Emanuel Peter]

On Mon, 13 Oct 2025 14:38:50 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This change refactor code that's similar for LShiftINode and
>> LShiftLNode into shared methods. I also added extra test cases to
>> cover all transformations.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
> 
>  - review
>  - Merge branch 'master' into JDK-8369167
>  - review
>  - sort headers
>  - more
>  - more
>  - more
>  - more
>  - more
>  - fix

Seems good to me, thanks for this cleanup @rwestrel ! I have only a few minor suggestions.

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

> 1080:       // Left input is an add of a constant?
> 1081:       const TypeInteger* t12 = phase->type(add1->in(2))->isa_integer(bt);
> 1082:       if (t12 && t12->is_con()) { // Left input is an add of a con?

Suggestion:

      if (t12 != nullptr && t12->is_con()) { // Left input is an add of a con?

Implicit null check not allowed by hotspot style guide, so we should fix it when we touch it ;)

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

> 1082:       if (t12 && t12->is_con()) { // Left input is an add of a con?
> 1083:         // Compute X << con0
> 1084:         Node *lsh = phase->transform(LShiftNode::make( add1->in(1), in(2), bt));

Suggestion:

        Node* lsh = phase->transform(LShiftNode::make(add1->in(1), in(2), bt));

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

> 1084:         Node *lsh = phase->transform(LShiftNode::make( add1->in(1), in(2), bt));
> 1085:         // Compute X<<con0 + (con1<<con0)
> 1086:         return AddNode::make( lsh, phase->integercon(java_shift_left(t12->get_con_as_long(bt), con, bt), bt), bt);

Suggestion:

        return AddNode::make(lsh, phase->integercon(java_shift_left(t12->get_con_as_long(bt), con, bt), bt), bt);

Fixing spaces

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

> 1187: Node* LShiftINode::Ideal(PhaseGVN *phase, bool can_reshape) {
> 1188:   return IdealIL(phase, can_reshape, T_INT);
> 1189: }

I fear that putting the comments here will make them go out of date quicker than putting them at `IdealIL`.

Seems the list here may not even be complete. What about this one? `((x >> C1) & Y) << C2` There could be more.

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

> 1223: 
> 1224:   uint shift = r2->get_con();
> 1225:   shift &= bits_per_java_integer(bt)-1;  // semantics of Java shifts

Suggestion:

  shift &= bits_per_java_integer(bt) - 1;  // semantics of Java shifts

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

> 1254: 
> 1255: //------------------------------Value------------------------------------------
> 1256: // A LShiftINode shifts its input2 left by input1 amount.

I would remove such a comment, it is rather useless here. If anything, such a comment belongs at the class definition.

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

> 1280: // Also collapse nested left-shifts with constant rhs:
> 1281: // (X << con1) << con2 ==> X << (con1 + con2)
> 1282: Node* LShiftLNode::Ideal(PhaseGVN* phase, bool can_reshape) {

Same here: comment will go out of sync because it is not close to the implementation. I would move it closer to the implementation.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27725#pullrequestreview-3334118372
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428076140
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428076683
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428079381
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428512329
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428518490
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428527472
PR Review Comment: https://git.openjdk.org/jdk/pull/27725#discussion_r2428531179