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

[Xin Liu]

On Sat, 20 Jan 2024 00:36:41 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:
> 
>   Assert for n2. Variables for n1/n2 opcode. More concise comments.
>   Overflow/random tests

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

> 354:   }
> 355: 
> 356:   bool is_int = n2->bottom_type()->isa_int() != nullptr;

I guess you change to n2 because n1 may be CmpI/L. 
I think it still works because TypeInt::CC is still isa_int(). or we add a comment to make it more clear?

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

> 380:     }
> 381:     phase->register_new_node(inv, phase->get_early_ctrl(inv));
> 382:     if (n1_is_cmp) {

CmpNode is subclass of SubNode. if n is CmpI/L, n->is_Sub() is also true. 
can we use the old logic for your new comparison expressions? 

yes, we still need to check if n1 is CmpNode or SubNode here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1460298766
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1460297868