RFR: 8303970: C2 can not merge homogeneous adjacent two If [v2]

[Tobias Hartmann]

On Mon, 13 Mar 2023 06:34:03 GMT, Yi Yang <yyang at openjdk.org> wrote:

>> Hi, can I have a review for this patch? It adds new Identity for BoolNode to lookup homogenous integer comparison, i.e. `Bool (CmpX a b)` is identity to `Bool (CmpX b a)`, in this way, we are able to merge the following two "identical" Ifs, which is not before.
>> 
>> 
>>     public static void test(int a, int b) { // ok, identical ifs, apply split_if
>>         if (a == b) {
>>             int_field = 0x42;
>>         } else {
>>             int_field = 42;
>>         }
>>         if (a == b) {
>>             int_field = 0x42;
>>         } else {
>>             int_field = 42;
>>         }
>>     }
>> 
>>     public static void test(int a, int b) { // do nothing
>>         if (a == b) {
>>             int_field = 0x42;
>>         } else {
>>             int_field = 42;
>>         }
>>         if (b == a) {
>>             int_field = 0x42;
>>         } else {
>>             int_field = 42;
>>         }
>>     }
>> 
>> 
>> Testing: tier1, appllication/ctw/modules
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   dont apply Identity for dead bool

src/hotspot/share/opto/subnode.cpp line 1470:

> 1468: }
> 1469: 
> 1470: static Node* get_reverse_cmp(int cmp_op, Node* cmp1, Node* cmp2) {

This should be a private member method to avoid the arguments. Please also add a comment.

src/hotspot/share/opto/subnode.cpp line 1487:

> 1485:   if (cop == Op_FastLock || cop == Op_FastUnlock ||
> 1486:       cop == Op_SubTypeCheck || cop == Op_VectorTest ||
> 1487:       cmp->is_Overflow()) {

Why is this check now needed?

src/hotspot/share/opto/subnode.cpp line 1495:

> 1493: Node* BoolNode::Identity(PhaseGVN* phase) {
> 1494:   // "Bool (CmpX a b)" is equivalent to "Bool (CmpX b a)"
> 1495:   Node *cmp = in(1);

Suggestion:

  Node* cmp = in(1);

src/hotspot/share/opto/subnode.cpp line 1502:

> 1500:     // During parsing, empty uses of bool is tolerable. During iterative GVN,
> 1501:     // we don't aggressively replace bool whose use is empty with existing node.
> 1502:     return this;

Why should we bother optimizing a dead Bool at all?

src/hotspot/share/opto/subnode.cpp line 1512:

> 1510:       Node* out = reverse_cmp->fast_out(i);
> 1511:       if (out->is_Bool() && out->as_Bool()->_test._test == _test._test &&
> 1512:           phase->type_or_null(out) != nullptr) {

Why is the `phase->type_or_null` required?

test/hotspot/jtreg/compiler/c2/irTests/TestBackToBackIfs.java line 1:

> 1: /*

Please add the bug id to the @bug statement.

test/hotspot/jtreg/compiler/c2/irTests/TestBackToBackIfs.java line 63:

> 61:     @Test
> 62:     @IR(counts = { IRNode.IF, "1" })
> 63:     public static void test1(int a, int b) {

Please add a test for `!=`.

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

PR: https://git.openjdk.org/jdk/pull/12978