RFR: 8356708: C2: loop strip mining expansion doesn't take sunk stores into account
Roberto Castañeda Lozano
rcastanedalo at openjdk.org
Wed Jun 11 14:25:32 UTC 2025
On Tue, 10 Jun 2025 10:17:11 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.
Looks good otherwise!
Another question: could `PhaseIdealLoop::try_move_store_before_loop` cause similar issues on strip-mined loops?
src/hotspot/share/opto/loopnode.cpp line 3026:
> 3024: // Do we already have a memory Phi for that slice on the outer loop? If that is the case, that Phi was created
> 3025: // by cloning an inner loop Phi. The inner loop Phi should have mem, the memory state of the first Store out of
> 3026: // the inner loop as input on the backedge. So does the outer loop Phi given it's a clone.
Suggestion:
// the inner loop, as input on the backedge. So does the outer loop Phi given it's a clone.
src/hotspot/share/opto/loopnode.cpp line 3043:
> 3041: igvn->replace_input_of(first, MemNode::Memory, phi);
> 3042: } else {
> 3043: // Fix memory state along the backedge: it should be the last sunk Stores of the chain
Suggestion:
// Fix memory state along the backedge: it should be the last sunk Store of the chain
test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java line 1:
> 1: /*
It would be good, for completeness, to add a "Couple stores sunk in outer loop, store in inner loop" test.
-------------
Changes requested by rcastanedalo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25717#pullrequestreview-2917408313
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2140301451
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2140302546
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2140308882
More information about the hotspot-compiler-dev
mailing list