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

[Roland Westrelin]

On Thu, 13 Mar 2025 09:31:04 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> Nice refactoring! I have a few small comments - mostly code style. Otherwise, looks good to me, too.

Thanks for the review.
New commit should address all your comments.
Now that the long min/max intrinsic is integrated, I also changed the long tests so they use long min/max and that triggered a bug in the code (missing `CONST64`) that I fixed.

> src/hotspot/share/opto/mulnode.cpp line 1361:
> 
>> 1359:     if (in(1)->Opcode() == Op_LShift(bt) &&
>> 1360:         in(1)->req() == 3 &&
>> 1361:         in(1)->in(2) == in(2)) {
> 
> Generally, is there notifaction code for this pattern to re-add the node to the IGVN worklist? If not, I don't think you need to handle it here if it's missing (it's just a missed opportunity but no correctness issue) but would be good to file a follow-up bug to handle it - especially when we want to add IGVN verification for `Ideal` and `Identity` with [JDK-8347273](https://bugs.openjdk.org/browse/JDK-8347273).

Good catch. I fixed a case that was handled for int but not for long. There are others that are missing for both AFAICT.
If I file a follow up bug, writing a test case for it is going to be very hard. So testing a fix is also hard. Shouldn't we wait for JDK-8347273 and fix whatever follows up of that?

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

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