RFR: 8323220: Reassociate loop invariants involved in Cmps and Add/Subs [v14]

[Christian Hagedorn]

On Fri, 22 Mar 2024 18:48:56 GMT, Joshua Cao <duke at openjdk.org> wrote:

>> // inv1 == (x + inv2) => ( inv1 - inv2 ) == x
>> // inv1 == (x - inv2) => ( inv1 + inv2 ) == x
>> // inv1 == (inv2 - x) => (-inv1 + inv2 ) == x
>> 
>> 
>> For example,
>> 
>> 
>> fn(inv1, inv2)
>>   while(...)
>>       x = foobar()
>>       if inv1 == x + inv2
>>            blackhole()
>> 
>> 
>> We can transform this into
>> 
>> 
>> fn(inv1, inv2)
>>   t = inv1 - inv2
>>   while(...)
>>       x = foobar()
>>       if t == x
>>            blackhole()
>> 
>> 
>> Here is an example: https://github.com/openjdk/jdk/blob/b78896b9aafcb15f453eaed6e154a5461581407b/src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java#L910. LHS `1` and RHS `pos` are both loop invariant
>> 
>> Passes tier1 locally on Linux machine. Passes GHA on my fork.
>
> Joshua Cao 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:
> 
>  - Merge branch 'master' into licm
>  - @run driver -> @run main
>  - Add tests for add/sub reassociation
>  - Merge branch 'master' into licm
>  - Make inputs deterministic. Make size an arg. Fix comments. Formatting.
>  - Update test to utilize @setup method for arguments
>  - Merge branch 'master' into licm
>  - Add correctness test for some random tests with random inputs
>  - Add some correctness tests where we do reassociate
>  - Remove unused TestInfo parameter. Have some tests exit mid-loop.
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/8daa6942...32cb9c0d

Only some minor comments. Otherwise, looks good to me, too!

src/hotspot/share/opto/loopTransform.cpp line 269:

> 267: }
> 268: 
> 269: //---------------------is_associative_cmp-------------------------

I usually remove these legacy headers when touching old code. I don't think they add any benefit nowadays.

src/hotspot/share/opto/loopTransform.cpp line 281:

> 279:     BoolNode* boolOut = n->out(i)->isa_Bool();
> 280:     if (boolOut == nullptr || !(boolOut->_test._test == BoolTest::eq ||
> 281:                                 boolOut->_test._test == BoolTest::ne)) {

We should use underscores instead of camelCase.
Suggestion:

    BoolNode* bool_out = n->out(i)->isa_Bool();
    if (bool_out == nullptr || !(bool_out->_test._test == BoolTest::eq ||
                                 bool_out->_test._test == BoolTest::ne)) {

src/hotspot/share/opto/loopTransform.cpp line 311:

> 309:         || op == Op_OrI  || op == Op_OrL
> 310:         || op == Op_XorI || op == Op_XorL
> 311:         || is_associative_cmp(n);

You should also update the comment on L304 which does not mention cmps.

src/hotspot/share/opto/loopTransform.cpp line 315:

> 313: }
> 314: 
> 315: //-------------------reassociate_add_sub_cmp---------------------

Can also be removed.

src/hotspot/share/opto/loopTransform.cpp line 335:

> 333: // inv1 == (x - inv2) => ( inv1 + inv2 ) == x
> 334: // inv1 == (inv2 - x) => (-inv1 + inv2 ) == x
> 335: //

Suggestion:

src/hotspot/share/opto/loopTransform.cpp line 353:

> 351:       (n2_is_sub && !n1_is_cmp && inv2_idx == 2) || (n1_is_cmp && !n2_is_sub);
> 352:   bool neg_inv1 =
> 353:       (n1_is_sub && inv1_idx == 2) || (n1_is_cmp && inv2_idx == 1 && n2_is_sub);

I suggest to remove the line breaks since the lines won't be longer than the added asserts above.

src/hotspot/share/opto/loopnode.hpp line 742:

> 740:   Node* reassociate(Node* n1, PhaseIdealLoop *phase);
> 741:   // Reassociate invariant add, subtract, and compare expressions.
> 742:   Node* reassociate_add_sub_cmp(Node* n1, int inv1_idx, int inv2_idx, PhaseIdealLoop *phase);

Suggestion:

  Node* reassociate_add_sub_cmp(Node* n1, int inv1_idx, int inv2_idx, PhaseIdealLoop* phase);

test/hotspot/jtreg/compiler/loopopts/InvariantCodeMotionReassociateAddSub.java line 36:

> 34:  * @summary Test loop invariant code motion of add/sub through reassociation
> 35:  * @library /test/lib /
> 36:  * @run main compiler.c2.loopopts.InvariantCodeMotionReassociateAddSub

You should use `driver` since we do not want to stress the driver VM with additionally passed VM flags like `-Xcomp` when running IR tests.
Suggestion:

 * @run driver compiler.c2.loopopts.InvariantCodeMotionReassociateAddSub

test/hotspot/jtreg/compiler/loopopts/InvariantCodeMotionReassociateCmp.java line 36:

> 34:  * @summary Test loop invariant code motion for cmp nodes through reassociation
> 35:  * @library /test/lib /
> 36:  * @run main compiler.c2.loopopts.InvariantCodeMotionReassociateCmp

Suggestion:

 * @run driver compiler.c2.loopopts.InvariantCodeMotionReassociateCmp

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

PR Review: https://git.openjdk.org/jdk/pull/17375#pullrequestreview-1982827351
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553439633
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553442818
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553444002
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553444134
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553444294
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553445604
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553447684
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553448796
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553453116