From 2d1b1096055f04be5d543357f1ac9b532a4f5d50 Mon Sep 17 00:00:00 2001 From: rwestrel Date: Thu, 12 Jun 2025 17:33:14 +0200 Subject: [PATCH] reviews --- src/hotspot/share/opto/loopnode.cpp | 80 +++++++++---------- .../TestStoresSunkInOuterStripMinedLoop.java | 24 ++++++ 2 files changed, 64 insertions(+), 40 deletions(-) diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index edcb1b7bfad..ae03c756c88 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/src/hotspot/share/opto/loopnode.cpp @@ -3007,50 +3007,50 @@ void OuterStripMinedLoopNode::handle_sunk_stores_at_expansion(PhaseIterGVN* igvn // 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(); - // 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); + if (!safepoint_mem->is_MergeMem()) { + assert(stores_in_outer_loop_cnt == 0, "inconsistent"); + return; + } + 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(); + // 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); + } + 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 (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 Store of the chain - igvn->replace_input_of(phi, LoopBackControl, last); - } + 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 Store 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"); } + assert(stores_in_outer_loop_cnt == stores_in_outer_loop_cnt2, "inconsistent"); } void OuterStripMinedLoopNode::adjust_strip_mined_loop(PhaseIterGVN* igvn) { diff --git a/test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java b/test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java index 21f57010c0a..da1cd2a198e 100644 --- a/test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java +++ b/test/hotspot/jtreg/compiler/loopstripmining/TestStoresSunkInOuterStripMinedLoop.java @@ -41,6 +41,7 @@ public class TestStoresSunkInOuterStripMinedLoop { public static void main(String[] args) { A a1 = new A(); A a2 = new A(); + A a3 = new A(); for (int i = 0; i < 20_000; i++) { field = 0; test1(); @@ -57,6 +58,11 @@ public class TestStoresSunkInOuterStripMinedLoop { if (a1.field != 1500) { throw new RuntimeException(a1.field + " != 1500"); } + a1.field = 0; + test4(a1, a2, a3); + if (a1.field != 1500) { + throw new RuntimeException(a1.field + " != 1500"); + } } } @@ -104,6 +110,24 @@ public class TestStoresSunkInOuterStripMinedLoop { return f; } + // Couples stores sunk in outer loop, store in inner loop + private static float test4(A a1, A a2, A a3) { + field = a1.field + a2.field + a3.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; + a3.field = v; + } + return f; + } + static class A { int field; }