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

[Emanuel Peter]

On Thu, 27 Feb 2025 16:57:27 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 17 additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8349361
>  - review
>  - review
>  - review
>  - Merge branch 'master' into JDK-8349361
>  - Update src/hotspot/share/opto/mulnode.cpp
>    
>    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>
>  - review
>  - Update src/hotspot/share/opto/mulnode.hpp
>    
>    Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth at users.noreply.github.com>
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/119a35df...d3b1cf08

@rwestrel Thanks for moving code so it is easier to review 😊 

I have a few more minor suggestions, but on the whole I'm happy with it :)

I'd still like to run another round of testing once we have it all finished up though, so please ping me again once you worked through my comments ;)

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

> 1399:   const Node* and_node = in(1);
> 1400:   if (and_node->Opcode() == Op_And(bt) &&
> 1401:       (mask_t = phase->type(and_node->in(2))->isa_integer(bt)) &&

I think this is an implicit nullptr check, right? It's not allowed according to OpenJDK style guide, but since it was here already I leave it to you if you want to fix it.

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

> 1429:   if (shift == 16 &&
> 1430:       (left_shift_t = phase->type(shl->in(2))->isa_int()) &&
> 1431:       left_shift_t->is_con(16)) {

Suggestion:

  const TypeInt* left_shift_t = phase->type(shl->in(2))->isa_int();
  if (shift == 16 &&
      left_shift_t != nullptr &&
      left_shift_t->is_con(16)) {

Would that be equivalent? I think it would be more readable.

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

> 1450:   // Check for "(byte[i] <<24)>>24" which simply sign-extends
> 1451:   if (shift == 24 &&
> 1452:       (left_shift_t = phase->type(shl->in(2))->isa_int()) &&

Then you could also refactor down here.
Because these are implicit nullptr checks again.

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

> 323: class RShiftNode : public Node {
> 324: public:
> 325:   RShiftNode(Node* in1, Node* in2) : Node(nullptr,in1,in2) {}

Suggestion:

  RShiftNode(Node* in1, Node* in2) : Node(nullptr, in1, in2) {}

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

> 334: class RShiftINode : public RShiftNode {
> 335: public:
> 336:   RShiftINode(Node* in1, Node* in2) : RShiftNode(in1,in2) {}

Suggestion:

  RShiftINode(Node* in1, Node* in2) : RShiftNode(in1, in2) {}

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

> 349: class RShiftLNode : public RShiftNode {
> 350: public:
> 351:   RShiftLNode(Node* in1, Node* in2) : RShiftNode(in1,in2) {}

Suggestion:

  RShiftLNode(Node* in1, Node* in2) : RShiftNode(in1, in2) {}

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

> 38:     }
> 39: 
> 40:     @Run(test = { "test1", "test2", "test3", "test4", "test5", "test6", "test7", "test8", "test9", "test10" })

For completeness sake, you should add the bug number above too ;)

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23438#pullrequestreview-2656343579
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978828495
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978834660
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978836066
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978846142
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978846474
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978846725
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1978849191