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

[Emanuel Peter]

On Fri, 7 Feb 2025 09:52:51 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 one additional commit since the last revision:
> 
>   review

@rwestrel nice work, looks like a good step to unify the code a little!

I left some comments / suggestions.

I'm also wondering about testing. How good do you think test coverage is? Are all cases covered? How about the edge-cases? Could we improve the coverage with randomization somehow?

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

> 1309: }
> 1310: 
> 1311: Node *RShiftNode::IdealIL(PhaseGVN* phase, bool can_reshape, BasicType bt) {

Suggestion:

Node* RShiftNode::IdealIL(PhaseGVN* phase, bool can_reshape, BasicType bt) {

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

> 1315:     return NodeSentinel;        // Left input is an integer
> 1316:   }
> 1317:   const TypeInteger* t3;  // type of in(1).in(2)

I know that you only moved this code, but it looks bad 🙈 
For one, why is it defined up here already when it is only used 10 lines later?
And why not give it a better name so we don't need the comment?
Suggestion:

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

> 1327:       (t3 = phase->type(mask->in(2))->isa_integer(bt)) &&
> 1328:       t3->is_con()) {
> 1329:     jlong maskbits = t3->get_con_as_long(bt);

This is also quite bad. It seems `mask` here is `in(1)`, which is not even the mask at all, but `x & <real_mask>`.

I'd suggest to clean it up a little and use better names.

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

> 1328:       t3->is_con()) {
> 1329:     jlong maskbits = t3->get_con_as_long(bt);
> 1330:     // Convert to "(x >> shift) & (mask >> shift)"

This is a nice comment. It could come as a motivation above. Because it suggests that we can then constant fold the `mask >> shift`, right?

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

> 1381: 
> 1382:   const TypeInteger* r1 = t1->isa_integer(bt); // Handy access
> 1383:   const TypeInt* r2 = t2->isa_int(); // Handy access

Suggestion:

  const TypeInteger* r1 = t1->isa_integer(bt);
  const TypeInt* r2 = t2->isa_int();

Let's reduce the noise a little.

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

> 1460:     return progress;
> 1461:   }
> 1462:   const TypeInt* t3;  // type of in(1).in(2)

Also refactor the use of `t3` here, please.

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

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

You should add the bug id above.

test/hotspot/jtreg/compiler/c2/irTests/RShiftLNodeIdealizationTests.java line 119:

> 117:     final int test7Shift = 42;
> 118:     final long test7Min = -1L << (64 - test7Shift -1);
> 119:     final long test7Max = ~test7Min;

Could we randomize these tests, so that we would get better coverage?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23438#pullrequestreview-2614514492
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954216080
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954225487
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954236168
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954235878
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954242414
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954253473
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954262666
PR Review Comment: https://git.openjdk.org/jdk/pull/23438#discussion_r1954264026