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

[Christian Hagedorn]

On Thu, 13 Mar 2025 07:56:10 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 two additional commits since the last revision:
> 
>  - Update test/hotspot/jtreg/compiler/c2/irTests/RShiftLNodeIdealizationTests.java
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>  - Update src/hotspot/share/opto/mulnode.cpp
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>

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

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).

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

> 1360:         in(1)->req() == 3 &&
> 1361:         in(1)->in(2) == in(2)) {
> 1362:       count &= bits_per_java_integer(bt)-1; // semantics of Java shifts

Suggestion:

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

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

> 1363:       // Compute masks for which this shifting doesn't change
> 1364:       jlong lo = (-1 << (bits_per_java_integer(bt) - ((uint)count)-1)); // FFFF8000
> 1365:       jlong hi = ~lo;               // 00007FFF

Seems strangely aligned. Maybe either align it to the comment above or convert to an unaligned comment.

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

> 1381: }
> 1382: 
> 1383: //------------------------------Ideal------------------------------------------

I think you can remove this line
Suggestion:

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

> 1396:   // and convert to (x >> 24) & (0xFF000000 >> 24) = x >> 24
> 1397:   // Such expressions arise normally from shift chains like (byte)(x >> 24).
> 1398:   const Node* and_node = in(1);

Same as above, do we have notification code for this patterns checking? Same for the patterns in `RShiftINode::Ideal()`.

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

> 1474:   if (t1 == TypeInteger::zero(bt)) return TypeInteger::zero(bt);
> 1475:   // Shift by zero does nothing
> 1476:   if (t2 == TypeInt::ZERO) return t1;

Can you add braces here for safety?

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

> 1488:   if (!r1->is_con() && r2->is_con()) {
> 1489:     uint shift = r2->get_con();
> 1490:     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 1509:

> 1507: #ifdef ASSERT
> 1508:     // Make sure we get the sign-capture idiom correct.
> 1509:     if (shift == bits_per_java_integer(bt)-1) {

Suggestion:

    if (shift == bits_per_java_integer(bt) - 1) {

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

> 322: 
> 323: class RShiftNode : public Node {
> 324: public:

Suggestion:

 public:

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

> 1700: }
> 1701: 
> 1702: const TypeInteger* TypeInteger::make(jlong lo, BasicType bt) {

Maybe you want to rename `lo` to `con` since we set `lo == hi`.

test/hotspot/jtreg/compiler/c2/irTests/RShiftINodeIdealizationTests.java line 27:

> 25: import jdk.test.lib.Asserts;
> 26: import compiler.lib.ir_framework.*;
> 27: 

Up to you if you want to update the copyright year or add your company's copyright. Same in the other test.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23438#pullrequestreview-2680958102
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993096261
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993018625
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993089087
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993019577
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993100333
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993016141
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993101084
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993101817
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993103866
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993108234
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1993110699