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

[Emanuel Peter]

On Fri, 5 Apr 2024 11:12:06 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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/71d3a1f1...32cb9c0d
>
> 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.

agree

> 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)) {

Agree, this is the C++ style. CamelCase is Java style.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553623796
PR Review Comment: https://git.openjdk.org/jdk/pull/17375#discussion_r1553624395