test & fix

src/hotspot/share/opto/loopnode.cpp

@@ -2870,10 +2870,11 @@ BaseCountedLoopNode* BaseCountedLoopNode::make(Node* entry, Node* backedge, Basi
   return new LongCountedLoopNode(entry, backedge);
 }
 
-void OuterStripMinedLoopNode::fix_sunk_stores(CountedLoopEndNode* inner_cle, LoopNode* inner_cl, PhaseIterGVN* igvn,
+void OuterStripMinedLoopNode::fix_sunk_stores_when_back_to_counted_loop(PhaseIterGVN* igvn,
-                                              PhaseIdealLoop* iloop) {
+                                                                        PhaseIdealLoop* iloop) const {
-  Node* cle_out = inner_cle->proj_out(false);
+  CountedLoopNode* inner_cl = inner_counted_loop();
-  Node* cle_tail = inner_cle->proj_out(true);
+  Node* cle_out = inner_loop_exit();
+
   if (cle_out->outcnt() > 1) {
     // Look for chains of stores that were sunk
     // out of the inner loop and are in the outer loop
@@ -2988,11 +2989,73 @@ void OuterStripMinedLoopNode::fix_sunk_stores(CountedLoopEndNode* inner_cle, Loo
   }
 }
 
+// Sunk stores should be referenced from an outer loop memory Phi
+void OuterStripMinedLoopNode::handle_sunk_stores_at_expansion(PhaseIterGVN* igvn) {
+  Node* cle_exit_proj = inner_loop_exit();
+
+  // Sunk stores are pinned on the loop exit projection of the inner loop
+#ifdef ASSERT
+  int stores_in_outer_loop_cnt = 0;
+  for (DUIterator_Fast imax, i = cle_exit_proj->fast_outs(imax); i < imax; i++) {
+    Node* u = cle_exit_proj->fast_out(i);
+    if (u->is_Store()) {
+      stores_in_outer_loop_cnt++;
+    }
+  }
+#endif
+
+  // Sunk stores are reachable from the memory state of the outer loop safepoint
+  Node* safepoint = outer_safepoint();
+  Node* safepoint_mem = safepoint->in(TypeFunc::Memory);
+  if (safepoint_mem->is_MergeMem()) {
+    MergeMemNode* mm = safepoint_mem->as_MergeMem();
+    DEBUG_ONLY(int stores_in_outer_loop_cnt2 = 0);
+    for (MergeMemStream mms(mm); mms.next_non_empty(); ) {
+      Node* mem = mms.memory();
+      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);
+      }
+      if (first != nullptr) {
+        // Found a chain of Stores that were sunk
+        // Do we already have a memory Phi for that slice on the outer loop? If that is the case, that Phi was created
+        // by cloning an inner loop Phi. The inner loop Phi should have mem, the memory state of the first Store out of
+        // the inner loop as input on the backedge. So does the outer loop Phi given it's a clone.
+        Node* phi = nullptr;
+        for (DUIterator_Fast imax, i = mem->fast_outs(imax); i < imax; i++) {
+          Node* u = mem->fast_out(i);
+          if (u->is_Phi() && u->in(0) == this && u->in(LoopBackControl) == mem) {
+            assert(phi == nullptr, "there should be only one");
+            phi = u;
+            PRODUCT_ONLY(break);
+          }
+        }
+        if (phi == nullptr) {
+          // No outer loop Phi? create one
+          phi = PhiNode::make(this, last);
+          phi->set_req(EntryControl, mem);
+          phi = igvn->transform(phi);
+          igvn->replace_input_of(first, MemNode::Memory, phi);
+        } else {
+          // Fix memory state along the backedge: it should be the last sunk Stores of the chain
+          igvn->replace_input_of(phi, LoopBackControl, last);
+        }
+      }
+    }
+    assert(stores_in_outer_loop_cnt == stores_in_outer_loop_cnt2, "inconsistent");
+  } else {
+    assert(stores_in_outer_loop_cnt == 0, "inconsistent");
+  }
+}
+
 void OuterStripMinedLoopNode::adjust_strip_mined_loop(PhaseIterGVN* igvn) {
   // Look for the outer & inner strip mined loop, reduce number of
   // iterations of the inner loop, set exit condition of outer loop,
   // construct required phi nodes for outer loop.
-  CountedLoopNode* inner_cl = unique_ctrl_out()->as_CountedLoop();
+  CountedLoopNode* inner_cl = inner_counted_loop();
   assert(inner_cl->is_strip_mined(), "inner loop should be strip mined");
   if (LoopStripMiningIter == 0) {
     remove_outer_loop_and_safepoint(igvn);
@@ -3010,7 +3073,7 @@ void OuterStripMinedLoopNode::adjust_strip_mined_loop(PhaseIterGVN* igvn) {
     inner_cl->clear_strip_mined();
     return;
   }
-  CountedLoopEndNode* inner_cle = inner_cl->loopexit();
+  CountedLoopEndNode* inner_cle = inner_counted_loop_end();
 
   int stride = inner_cl->stride_con();
   // For a min int stride, LoopStripMiningIter * stride overflows the int range for all values of LoopStripMiningIter
@@ -3111,6 +3174,8 @@ void OuterStripMinedLoopNode::adjust_strip_mined_loop(PhaseIterGVN* igvn) {
     }
   }
 
+  handle_sunk_stores_at_expansion(igvn);
+
   if (iv_phi != nullptr) {
     // Now adjust the inner loop's exit condition
     Node* limit = inner_cl->limit();
@@ -3166,7 +3231,7 @@ void OuterStripMinedLoopNode::transform_to_counted_loop(PhaseIterGVN* igvn, Phas
   CountedLoopEndNode* inner_cle = inner_cl->loopexit();
   Node* safepoint = outer_safepoint();
 
-  fix_sunk_stores(inner_cle, inner_cl, igvn, iloop);
+  fix_sunk_stores_when_back_to_counted_loop(igvn, iloop);
 
   // make counted loop exit test always fail
   ConINode* zero = igvn->intcon(0);

src/hotspot/share/opto/loopnode.hpp

@@ -573,7 +573,8 @@ class LoopLimitNode : public Node {
 // Support for strip mining
 class OuterStripMinedLoopNode : public LoopNode {
 private:
-  static void fix_sunk_stores(CountedLoopEndNode* inner_cle, LoopNode* inner_cl, PhaseIterGVN* igvn, PhaseIdealLoop* iloop);
+  void fix_sunk_stores_when_back_to_counted_loop(PhaseIterGVN* igvn, PhaseIdealLoop* iloop) const;
+  void handle_sunk_stores_at_expansion(PhaseIterGVN* igvn);
 
 public:
   OuterStripMinedLoopNode(Compile* C, Node *entry, Node *backedge)
@@ -589,6 +590,10 @@ public:
   virtual OuterStripMinedLoopEndNode* outer_loop_end() const;
   virtual IfFalseNode* outer_loop_exit() const;
   virtual SafePointNode* outer_safepoint() const;
+  CountedLoopNode* inner_counted_loop() const { return unique_ctrl_out()->as_CountedLoop(); }
+  CountedLoopEndNode* inner_counted_loop_end() const { return  inner_counted_loop()->loopexit(); }
+  IfFalseNode* inner_loop_exit() const { return inner_counted_loop_end()->proj_out(false)->as_IfFalse(); }
+
   void adjust_strip_mined_loop(PhaseIterGVN* igvn);
 
   void remove_outer_loop_and_safepoint(PhaseIterGVN* igvn) const;

test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2025, Red Hat, Inc. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8356708
+ * @summary C2: loop strip mining expansion doesn't take sunk stores into account
+ *
+ * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -XX:LoopMaxUnroll=0
+ *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM -XX:StressSeed=26601954 TestStoresSunkInOuterStripMinedLoop
+ * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -XX:LoopMaxUnroll=0
+ *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM TestStoresSunkInOuterStripMinedLoop
+ * @run main TestStoresSunkInOuterStripMinedLoop
+ *
+ */
+
+public class TestStoresSunkInOuterStripMinedLoop {
+    private static int field;
+    private static volatile int volatileField;
+
+    public static void main(String[] args) {
+        A a1 = new A();
+        A a2 = new A();
+        for (int i = 0; i < 20_000; i++) {
+            field = 0;
+            test1();
+            if (field != 1500) {
+                throw new RuntimeException(field + " != 1500");
+            }
+            a1.field = 0;
+            test2(a1, a2);
+            if (a1.field != 1500) {
+                throw new RuntimeException(a1.field + " != 1500");
+            }
+            a1.field = 0;
+            test3(a1, a2);
+            if (a1.field != 1500) {
+                throw new RuntimeException(a1.field + " != 1500");
+            }
+        }
+    }
+
+    // Single store sunk in outer loop, no store in inner loop
+    private static float test1() {
+        int v = field;
+        float f = 1;
+        for (int i = 0; i < 1500; i++) {
+            f *= 2;
+            v++;
+            field = v;
+        }
+        return f;
+    }
+
+    // Couple stores sunk in outer loop, no store in inner loop
+    private static float test2(A a1, A a2) {
+        field = a1.field + a2.field;
+        volatileField = 42;
+        int v = a1.field;
+        float f = 1;
+        for (int i = 0; i < 1500; i++) {
+            f *= 2;
+            v++;
+            a1.field = v;
+            a2.field = v;
+        }
+        return f;
+    }
+
+    // Store sunk in outer loop, store in inner loop
+    private static float test3(A a1, A a2) {
+        field = a1.field + a2.field;
+        volatileField = 42;
+        int v = a1.field;
+        float f = 1;
+        A a = a2;
+        for (int i = 0; i < 1500; i++) {
+            f *= 2;
+            v++;
+            a.field = v;
+            a = a1;
+            a2.field = v;
+        }
+        return f;
+    }
+
+    static class A {
+        int field;
+    }
+}