RFR: 8369435: C2: transform (LShiftX (SubX con0 a), con1) into (SubX con0<<con1 (LShiftX a con1)) [v2]

[Benoît Maillard]

On Thu, 16 Oct 2025 13:18:01 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> We already transform:
>> 
>> (LShiftX (AddX a con0), con1) into (AddX (LShiftX a con1) con0<<con1)
>> 
>> THis is a variant with SubX. I found that this helps RCE.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review

Thanks for making this change @rwestrel! Looks good to me, I only have nits regarding the comments.

I have submitted testing and will come back with the results.

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

> 1089:     }
> 1090:   }
> 1091:   // Transform is legal, but check for profit.  Avoid breaking 'i2s'

I would add a comment that explicitly states the pattern we are looking at, this makes reading the code much faster imo:
Suggestion:

  // Check for "(con0 - X) << con1" 
  // Transform is legal, but check for profit.  Avoid breaking 'i2s'

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

> 1097:       // Compute X << con0
> 1098:       Node* lsh = phase->transform(LShiftNode::make(add1->in(2), in(2), bt));
> 1099:       // Compute X<<con0 - (con1<<con0)

I think this is in the wrong order
Suggestion:

      // Compute (con1<<con0) - (X<<con0)

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

PR Review: https://git.openjdk.org/jdk/pull/27842#pullrequestreview-3350854308
PR Review Comment: https://git.openjdk.org/jdk/pull/27842#discussion_r2440398423
PR Review Comment: https://git.openjdk.org/jdk/pull/27842#discussion_r2440405909