From a0d7442c27eca09dee6cf45a58c449e778fa7aea Mon Sep 17 00:00:00 2001 From: rwestrel Date: Mon, 2 Jun 2025 17:29:23 +0200 Subject: [PATCH] test & fix --- src/hotspot/share/opto/loopnode.cpp | 79 +++++++++++-- src/hotspot/share/opto/loopnode.hpp | 7 +- .../TestStoresSunkInOuterStripMinedLoop.java | 110 ++++++++++++++++++ 3 files changed, 188 insertions(+), 8 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index 195d48b37c6..a0a0b1e440d 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/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, - PhaseIdealLoop* iloop) { - Node* cle_out = inner_cle->proj_out(false); - Node* cle_tail = inner_cle->proj_out(true); +void OuterStripMinedLoopNode::fix_sunk_stores_when_back_to_counted_loop(PhaseIterGVN* igvn, + PhaseIdealLoop* iloop) const { + CountedLoopNode* inner_cl = inner_counted_loop(); + 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); diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index 0f87ed6763d..cb5468bfd02 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/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; diff --git a/test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java b/test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java new file mode 100644 index 00000000000..21f57010c0a --- /dev/null +++ b/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; + } +}