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

[Roberto Castañeda Lozano]

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.

Thanks for working on this, Roland. I just submitted some testing, will come back with the results in a day or two.

Generally, I agree with your proposed approach of handling the case at expansion time as a low-risk fix for JDK 25. But as future work, would it be feasible to maintain regular SSA form for outer strip-mined loops (adding memory and data phi nodes at both loop levels) rather than omitting phi nodes for the outer loops and "repairing" SSA on macro expansion, or is there any fundamental obstacle in doing the former? It would have prevented issues like this, and feels like a more principled and robust approach in general.

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

PR Comment: https://git.openjdk.org/jdk/pull/25717#issuecomment-2960287377