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

[Roland Westrelin]

On Tue, 10 Jun 2025 18:51:49 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> 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.

I don't think there's a fundamental obstacle. The reason I implemented loop strip mining that way is that I was concerned it would be complicated for existing transformations to be supported without major code change and that there was a chance subtle issues would creep in (such as some optimizations not happening any more). So I tried to have a minimal set of extra nodes for strip mined loops initially so existing transformations would simply need to skip over the `OuterStripMinedLoop`. 

It's quite possible that having the full outer strip mined loop early on works fine and that there's no need for changes all over the loop optimizations. I suppose someone would need to give it a try. This said, I still think keeping the graph simple when crucial transformations happen has some merit.

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

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