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

[Emanuel Peter]

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.

@rwestrel Thanks for looking into this! The fix seems reasonable given that we don't have phi's at the outer loop. But why don't we have those phis in the first place?

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

> 3008:   Node* safepoint = outer_safepoint();
> 3009:   Node* safepoint_mem = safepoint->in(TypeFunc::Memory);
> 3010:   if (safepoint_mem->is_MergeMem()) {

I would have flipped the condition, and made an early exit condition from this. That way, the code is indented one level less. Just a suggestion, feel free to ignore :)

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

> 3019:         first = mem;
> 3020:         mem = mem->in(MemNode::Memory);
> 3021:       }

Suggestion:

      // Traverse up the chain of stores to find the first store pinned
      // at the loop exit projection.
      Node* last = mem;
      Node* first = nullptr;
      while (mem->is_Store() && mem->in(0) == cle_exit_proj) {
        DEBUG_ONLY(stores_in_outer_loop_cnt2++);
        first = mem;
        mem = mem->in(MemNode::Memory);
      }

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

PR Review: https://git.openjdk.org/jdk/pull/25717#pullrequestreview-2920838151
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2142549734
PR Review Comment: https://git.openjdk.org/jdk/pull/25717#discussion_r2142554505