RFR: 8356708: C2: loop strip mining expansion doesn't take sunk stores into account [v4]

[Roland Westrelin]

On Tue, 17 Jun 2025 07:14:51 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> I see you have some minimal comments at `PhaseIdealLoop::create_outer_strip_mined_loop`:
> 
> ```
> // to the loop head. The inner strip mined loop is left as it is. Only
> // once loop optimizations are over, do we adjust the inner loop exit
> // condition to limit its number of iterations, set the outer loop
> // exit condition and add Phis to the outer loop head.
> ```
> 
> I think we should add some more info there, and link to and from `OuterStripMinedLoopNode::handle_sunk_stores_at_expansion`.

Done in new commit. Can you have another look @eme64 ?

> src/hotspot/share/opto/loopnode.cpp line 2992:
> 
>> 2990: }
>> 2991: 
>> 2992: // Sunk stores should be referenced from an outer loop memory Phi
> 
> I think you really need to give some longer explanation here why we need to do what you do here.
> 
> Also: is there anywhere a description why we do not have the phi already by default for outer loops? Because I think we should really describe that somewhere, and state our assumptions. And then you could also refer to that description from here, and from there to here.

Updated in new commit.

> src/hotspot/share/opto/loopnode.cpp line 2993:
> 
>> 2991: 
>> 2992: // Sunk stores should be referenced from an outer loop memory Phi
>> 2993: void OuterStripMinedLoopNode::handle_sunk_stores_at_expansion(PhaseIterGVN* igvn) {
> 
> What does the word "expansion" refer to? Could you also mention that in your code comment above, please?

I renamed that one.

> src/hotspot/share/opto/loopnode.cpp line 2996:
> 
>> 2994:   Node* cle_exit_proj = inner_loop_exit();
>> 2995: 
>> 2996:   // Sunk stores are pinned on the loop exit projection of the inner loop
> 
> Can you add a description why?

New commit should address that one as well.

> src/hotspot/share/opto/loopnode.cpp line 3007:
> 
>> 3005: #endif
>> 3006: 
>> 3007:   // Sunk stores are reachable from the memory state of the outer loop safepoint
> 
> Is it true that the control of the safepoint is the `cle_exit_proj`? Could we add an assert for that? So we are just looking for all memory between those two control nodes? Or is it more complicated?

Yes, it is. I added a call to `verify_strip_mined()` that checks the shape of the outer loop including the control flow nodes.

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

PR Comment: https://git.openjdk.org/jdk/pull/25717#issuecomment-2980261940
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2152184342
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2152190214
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2152185916
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2152189746