RFR: 8356708: C2: loop strip mining expansion doesn't take sunk stores into account [v3]
Roberto Castañeda Lozano
rcastanedalo at openjdk.org
Fri Jun 13 12:45:31 UTC 2025
On Thu, 12 Jun 2025 15:38:53 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 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 six additional commits since the last revision:
>
> - reviews
> - Merge branch 'master' into JDK-8356708
> - Update src/hotspot/share/opto/loopnode.cpp
>
> Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
> - Update src/hotspot/share/opto/loopnode.cpp
>
> Co-authored-by: Roberto Castañeda Lozano <robcasloz at users.noreply.github.com>
> - Update src/hotspot/share/opto/loopnode.cpp
>
> Co-authored-by: Roberto Castañeda Lozano <robcasloz at users.noreply.github.com>
> - test & fix
Testing went faster than I thought and did not reveal any issue, looks good, thank you for fixing this!
test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java line 81:
> 79: }
> 80:
> 81: // Couple stores sunk in outer loop, no store in inner loop
Suggestion:
// Multiple stores sunk in outer loop, no store in inner loop
test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java line 113:
> 111: }
> 112:
> 113: // Couples stores sunk in outer loop, store in inner loop
Suggestion:
// Multiple stores sunk in outer loop, store in inner loop
-------------
Marked as reviewed by rcastanedalo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25717#pullrequestreview-2924671681
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2145011733
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2145012350
More information about the hotspot-compiler-dev
mailing list