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

[Emanuel Peter]

On Fri, 1 Mar 2024 00:24:45 GMT, Joshua Cao <duke at openjdk.org> wrote:

>> @caojoshua it looks really good now!
>> I'm running our internal testing again, will report back.
>> 
>> One more concern I just had: do we have tests for the pre-existing Add/Sub reassociations? Because you now touched the logic around there we should make sure there are at least correctness tests. IR tests are probably basically impossible because it is the same number of Add/Sub nodes before and after the optimization.
>> 
>> I'd like to have another Reviewer look over this as well, therefore:
>
>> One more concern I just had: do we have tests for the pre-existing Add/Sub reassociations?
> 
> Not that I know of. A bunch of reassociation was added in https://github.com/openjdk/jdk/commit/23ed3a9e91ac57295d274fefdf6c0a322b1e87b7, which does not have any tests.
> 
> I ran `make CONF=linux-x86_64-server-fastdebug test TEST=all TEST_VM_OPTS=-XX:-TieredCompilation` on my Linux machine. I have 4 failures in `SctpChannel` and 3 failures in `CAInterop.java`, but they also fail on master branch so they should not be caused by this patch. Hopefully this adds a little more confidence.

@caojoshua 

I also ran our internal testing and it looks ok (only unrelated failures). But of course that is only on tests that we have, and if the other reassociations are not tested, then that helps little ;)

> Not that I know of. A bunch of reassociation was added in https://github.com/openjdk/jdk/commit/23ed3a9e91ac57295d274fefdf6c0a322b1e87b7, which does not have any tests.

Could you please add a result verification test per case of pre-existing reassociation? Otherwise I'm afraid it is hard to be sure you did not break those cases.

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

PR Comment: https://git.openjdk.org/jdk/pull/17375#issuecomment-1972548903