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

[Emanuel Peter]

On Sat, 27 Jan 2024 00:11:50 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 incrementally with one additional commit since the last revision:
> 
>   Small fixes and add check methods for tests

I thought the `TestInfo info` was optional, so if you don't use it, then don't put it in the arguments ;)

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

> 113:             }
> 114:         }
> 115:     }

I suggest you add a test that first iterates some number of iterations, and then returns inside the if-statement with return value i. Then you check if that value is as expected.
Because with all the tests that you have here, I'm not sure if really all branches are always taken, or if they may lead to uncommon traps, and hence leave some things uncovered by testing.

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

> 202: 
> 203:     @Check(test = "leDontReassociate")
> 204:     public void checkLeDontReassociate(int returnValue, TestInfo info) {

Suggestion:

    public void checkLeDontReassociate(int returnValue) {

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

> 223: 
> 224:     @Check(test = "gtDontReassociate")
> 225:     public void checkGtDontReassociate(int returnValue, TestInfo info) {

Suggestion:

    public void checkGtDontReassociate(int returnValue) {

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

> 244: 
> 245:     @Check(test = "geDontReassociate")
> 246:     public void checkGeDontReassociate(int returnValue, TestInfo info) {

Suggestion:

    public void checkGeDontReassociate(int returnValue) {

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17375#pullrequestreview-1855666684
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1473878899
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1473874562
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1473874367
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1473874215