RFR: 8281322: C2: always construct strip mined loop initially (even if strip mining is disabled) [v4]

[Christian Hagedorn]

On Mon, 7 Mar 2022 17:18:39 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Some of the long range check transformations take advantage of the
>> safepoint captured by loop strip mining to extract jvm state (in order
>> to add back empty predicates to the inner loop of a loop nest). As a
>> consequence, irTests/TestLongRangeChecks.java fails with strip mining
>> off and users might experience performance anomalies where changing
>> GCs affect purely computational code.
>> 
>> The strip mined loop nest creation is a 2 step process:
>> 
>> 1- when a CountedLoop is created, an OuterStripMinedLoop is also added
>>   but it's not fully constructed
>> 
>> 2- at macro expansion time, the OuterStripMinedLoop is turned into an
>>   actual loop by adding Phis and a proper exit condition
>> 
>> I propose always doing 1- whether loop strip mining is enabled or
>> not. This causes the safepoint to always be captured. Loop strip ming
>> is not expected to get in the way of loop transformations so this
>> change in itself should be performance neutral. Then at 2-, if loop
>> strip mining is not enabled, the OuterStripMinedLoop can be removed
>> and the safepoint moved back into the loop in case
>> LoopStripMiningIter=1 or simply removed too.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   build fix

Apart from some minor code style things, the update looks good to me!

src/hotspot/share/opto/loopnode.cpp line 2567:

> 2565: }
> 2566: 
> 2567: void OuterStripMinedLoopNode::fix_sunk_stores(CountedLoopEndNode* inner_cle, LoopNode* target, PhaseIterGVN* igvn,

I suggest to rename `target` to `inner_cl` as we are also using `inner_cle` already. Maybe you can also add some more comments for the intermediate steps (even though most of the code was just moved in this change).

src/hotspot/share/opto/loopnode.cpp line 2579:

> 2577:         int alias_idx = igvn->C->get_alias_index(u->adr_type());
> 2578:         Node* first = u;
> 2579:         for(;;) {

Missing space after `for`.

src/hotspot/share/opto/loopnode.cpp line 2588:

> 2586:         }
> 2587:         Node* last = u;
> 2588:         for(;;) {

Missing space after `for`.

src/hotspot/share/opto/loopnode.cpp line 2651:

> 2649: #endif
> 2650:         if (phi == NULL) {
> 2651:           // If the an entire chains was sunk, the

`If the an entire chains` -> `If an entire chain`

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7364