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

[Emanuel Peter]

On Tue, 17 Jun 2025 12:53:48 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> `test1()` has a counted loop with a `Store` to `field`. That `Store`
>> is sunk out of loop. When the `OuterStripMinedLoop` is expanded, only
>> `Phi`s that exist at the inner loop are added to the outer
>> loop. There's no `Phi` for the slice of the sunk `Store` (because
>> there's no `Store` left in the inner loop) so no `Phi` is added for
>> that slice to the outer loop. As a result, there's a missing anti
>> dependency for `Load` of `field` that's before the loop and it can be
>> scheduled inside the outer strip mined loop which is incorrect.
>> 
>> `test2()` is the same as `test1()` but with a chain of 2 `Store`s.
>> 
>> `test3()` is another variant where a `Store` is left in the inner loop
>> after one is sunk out of it so the inner loop still has a `Phi`. As a
>> result, the outer loop also gets a `Phi` but it's incorrectly wired as
>> the sunk `Store` should be the input along the backedge but is
>> not. That one doesn't cause any failure AFAICT.
>> 
>> The fix I propose is some extra logic at expansion of the
>> `OuterStripMinedLoop` to handle these corner cases.
>
> Roland Westrelin has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - review
>  - more

Thanks for the updates an improved documentation!

I have a few more minor suggestions :)

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

> 331: //
> 332: // As loop optimizations transform the inner loop, the outer strip mined loop stays mostly unchanged. The only exception
> 333: // is nodes referenced from the SafePoint and sunk from the inner loop: they end up in the outer strip mined loop.

Do you want to reference `handle_sunk_stores_when_finishing_construction`?

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

> 336: // to be mostly unaffected by the outer strip mined loop: the only extra step needed in most cases is to step over the
> 337: // OuterStripMinedLoop. The main drawback is that once loop optimizations are over, an extra step is needed to finish
> 338: // constructing the outer loop. This is handled by OuterStripMinedLoopNode::adjust_strip_mined_loop().

You should probably say explicitly which C2 IR rule we violate: whenever a memory slice is mutated in a loop, there needs to be a corresponding phi.

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

> 3022: // for each chain of sunk Stores for a particular memory slice. If some Stores were sunk and some left in the inner loop,
> 3023: // a Phi was already created in the outer loop but its backedge input wasn't wired correctly to the last Store of the
> 3024: // chain.

Suggestion:

// We're now in the process of finishing the construction of the outer loop. For each Phi in the inner loop, a Phi in
// the outer loop was just now created. However, Sunk Stores cause an extra challenge:
// 1) If all Stores in the inner loop were sunk for a particular memory slice, there's no Phi left for that memory
//    slice in the inner loop any more, and hence we did not yet add a Phi for the outer loop. So an extra Phi
//    must now be added for each chain of sunk Stores for a particular memory slice.
// 2) If some Stores were sunk and some left in the inner loop, a Phi was already created in the outer loop but
//    its backedge input wasn't wired correctly to the last Store of the chain. We had wired the memory state at
//    the inner loop exit to the Phi backedge, but we should have taken the last Store of the chain instead. We
//    will now have to fix that too.

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

> 3214:   }
> 3215: 
> 3216:   handle_sunk_stores_when_finishing_construction(igvn);

Above, where you insert the phis, you may want to say something about the case of Sunk Stores as well.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25717#pullrequestreview-2937907190
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2153762057
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2153765691
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2153826690
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2153822355