diff --git a/src/hotspot/share/opto/arraycopynode.cpp b/src/hotspot/share/opto/arraycopynode.cpp index 23a6ee31fff..85b6bd21aec 100644 --- a/src/hotspot/share/opto/arraycopynode.cpp +++ b/src/hotspot/share/opto/arraycopynode.cpp @@ -681,18 +681,21 @@ bool ArrayCopyNode::may_modify(const TypeOopPtr* t_oop, PhaseValues* phase) { return CallNode::may_modify_arraycopy_helper(dest_t, t_oop, phase); } -bool ArrayCopyNode::may_modify_helper(const TypeOopPtr* t_oop, Node* n, PhaseValues* phase, CallNode*& call) { +bool ArrayCopyNode::may_modify_helper(const TypeOopPtr* t_oop, Node* n, PhaseValues* phase, ArrayCopyNode*& ac) { if (n != nullptr && - n->is_Call() && - n->as_Call()->may_modify(t_oop, phase) && - (n->as_Call()->is_ArrayCopy() || n->as_Call()->is_call_to_arraycopystub())) { - call = n->as_Call(); + n->is_ArrayCopy() && + n->as_ArrayCopy()->may_modify(t_oop, phase)) { + ac = n->as_ArrayCopy(); return true; } return false; } bool ArrayCopyNode::may_modify(const TypeOopPtr* t_oop, MemBarNode* mb, PhaseValues* phase, ArrayCopyNode*& ac) { + if (mb->trailing_expanded_array_copy()) { + return true; + } + Node* c = mb->in(0); BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2(); @@ -705,23 +708,19 @@ bool ArrayCopyNode::may_modify(const TypeOopPtr* t_oop, MemBarNode* mb, PhaseVal for (uint i = 1; i < c->req(); i++) { if (c->in(i) != nullptr) { Node* n = c->in(i)->in(0); - if (may_modify_helper(t_oop, n, phase, call)) { - ac = call->isa_ArrayCopy(); + if (may_modify_helper(t_oop, n, phase, ac)) { assert(c == mb->in(0), "only for clone"); return true; } } } - } else if (may_modify_helper(t_oop, c->in(0), phase, call)) { - ac = call->isa_ArrayCopy(); + } else if (may_modify_helper(t_oop, c->in(0), phase, ac)) { #ifdef ASSERT bool use_ReduceInitialCardMarks = BarrierSet::barrier_set()->is_a(BarrierSet::CardTableBarrierSet) && static_cast(bs)->use_ReduceInitialCardMarks(); - assert(c == mb->in(0) || (ac != nullptr && ac->is_clonebasic() && !use_ReduceInitialCardMarks), "only for clone"); + assert(c == mb->in(0) || (ac->is_clonebasic() && !use_ReduceInitialCardMarks), "only for clone"); #endif return true; - } else if (mb->trailing_partial_array_copy()) { - return true; } return false; diff --git a/src/hotspot/share/opto/arraycopynode.hpp b/src/hotspot/share/opto/arraycopynode.hpp index f792722068f..13e739fc2c7 100644 --- a/src/hotspot/share/opto/arraycopynode.hpp +++ b/src/hotspot/share/opto/arraycopynode.hpp @@ -121,7 +121,7 @@ private: BasicType copy_type, const Type* value_type, int count); bool finish_transform(PhaseGVN *phase, bool can_reshape, Node* ctl, Node *mem); - static bool may_modify_helper(const TypeOopPtr* t_oop, Node* n, PhaseValues* phase, CallNode*& call); + static bool may_modify_helper(const TypeOopPtr* t_oop, Node* n, PhaseValues* phase, ArrayCopyNode*& ac); public: static Node* load(BarrierSetC2* bs, PhaseGVN *phase, Node*& ctl, MergeMemNode* mem, Node* addr, const TypePtr* adr_type, const Type *type, BasicType bt); private: diff --git a/src/hotspot/share/opto/macro.hpp b/src/hotspot/share/opto/macro.hpp index e10326e9d87..0e2326fd9d4 100644 --- a/src/hotspot/share/opto/macro.hpp +++ b/src/hotspot/share/opto/macro.hpp @@ -111,7 +111,7 @@ private: void expand_unlock_node(UnlockNode *unlock); // More helper methods modeled after GraphKit for array copy - void insert_mem_bar(Node** ctrl, Node** mem, int opcode, Node* precedent = nullptr); + void insert_mem_bar(Node** ctrl, Node** mem, int opcode, int alias_idx, Node* precedent = nullptr); Node* array_element_address(Node* ary, Node* idx, BasicType elembt); Node* ConvI2L(Node* offset); @@ -171,7 +171,7 @@ private: Node* src, Node* src_offset, Node* dest, Node* dest_offset, Node* copy_length, bool dest_uninitialized); - bool generate_unchecked_arraycopy(Node** ctrl, MergeMemNode** mem, + void generate_unchecked_arraycopy(Node** ctrl, MergeMemNode** mem, const TypePtr* adr_type, BasicType basic_elem_type, bool disjoint_bases, diff --git a/src/hotspot/share/opto/macroArrayCopy.cpp b/src/hotspot/share/opto/macroArrayCopy.cpp index e37c961e88d..e2209fddbdf 100644 --- a/src/hotspot/share/opto/macroArrayCopy.cpp +++ b/src/hotspot/share/opto/macroArrayCopy.cpp @@ -36,8 +36,8 @@ #include "utilities/align.hpp" #include "utilities/powerOfTwo.hpp" -void PhaseMacroExpand::insert_mem_bar(Node** ctrl, Node** mem, int opcode, Node* precedent) { - MemBarNode* mb = MemBarNode::make(C, opcode, Compile::AliasIdxBot, precedent); +void PhaseMacroExpand::insert_mem_bar(Node** ctrl, Node** mem, int opcode, int alias_idx, Node* precedent) { + MemBarNode* mb = MemBarNode::make(C, opcode, alias_idx, precedent); mb->init_req(TypeFunc::Control, *ctrl); mb->init_req(TypeFunc::Memory, *mem); transform_later(mb); @@ -45,7 +45,14 @@ void PhaseMacroExpand::insert_mem_bar(Node** ctrl, Node** mem, int opcode, Node* transform_later(*ctrl); Node* mem_proj = new ProjNode(mb,TypeFunc::Memory); transform_later(mem_proj); - *mem = mem_proj; + if (alias_idx == Compile::AliasIdxBot) { + *mem = mem_proj; + } else { + MergeMemNode* mm = (*mem)->clone()->as_MergeMem(); + mm->set_memory_at(alias_idx, mem_proj); + transform_later(mm); + *mem = mm; + } } Node* PhaseMacroExpand::array_element_address(Node* ary, Node* idx, BasicType elembt) { @@ -667,16 +674,15 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode* } } - bool is_partial_array_copy = false; if (!(*ctrl)->is_top()) { // Generate the fast path, if possible. Node* local_ctrl = *ctrl; MergeMemNode* local_mem = MergeMemNode::make(mem); transform_later(local_mem); - is_partial_array_copy = generate_unchecked_arraycopy(&local_ctrl, &local_mem, - adr_type, copy_type, disjoint_bases, - src, src_offset, dest, dest_offset, - ConvI2X(copy_length), acopy_to_uninitialized); + generate_unchecked_arraycopy(&local_ctrl, &local_mem, + adr_type, copy_type, disjoint_bases, + src, src_offset, dest, dest_offset, + ConvI2X(copy_length), acopy_to_uninitialized); // Present the results of the fast call. result_region->init_req(fast_path, local_ctrl); @@ -818,16 +824,23 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode* // Do not let stores that initialize this object be reordered with // a subsequent store that would make this object accessible by // other threads. - insert_mem_bar(ctrl, &out_mem, Op_MemBarStoreStore); + assert(ac->_dest_type == TypeOopPtr::BOTTOM, "non escaping destination shouldn't have narrow slice"); + insert_mem_bar(ctrl, &out_mem, Op_MemBarStoreStore, Compile::AliasIdxBot); } else { - insert_mem_bar(ctrl, &out_mem, Op_MemBarCPUOrder); + int mem_bar_alias_idx = Compile::AliasIdxBot; + if (ac->_dest_type != TypeOopPtr::BOTTOM) { + // The graph was transformed under the assumption the ArrayCopy node only had an effect on a narrow slice. We can't + // insert a wide membar now that it's being expanded: a load that uses the input memory state of the ArrayCopy + // could then become anti dependent on the membar when it was not anti dependent on the ArrayCopy leading to a + // broken graph. + mem_bar_alias_idx = C->get_alias_index(ac->_dest_type->add_offset(Type::OffsetBot)->is_ptr()); + } + insert_mem_bar(ctrl, &out_mem, Op_MemBarCPUOrder, mem_bar_alias_idx); } - if (is_partial_array_copy) { - assert((*ctrl)->is_Proj(), "MemBar control projection"); - assert((*ctrl)->in(0)->isa_MemBar(), "MemBar node"); - (*ctrl)->in(0)->isa_MemBar()->set_trailing_partial_array_copy(); - } + assert((*ctrl)->is_Proj(), "MemBar control projection"); + assert((*ctrl)->in(0)->isa_MemBar(), "MemBar node"); + (*ctrl)->in(0)->isa_MemBar()->set_trailing_expanded_array_copy(); _igvn.replace_node(_callprojs.fallthrough_memproj, out_mem); if (_callprojs.fallthrough_ioproj != nullptr) { @@ -837,7 +850,7 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode* #ifdef ASSERT const TypeOopPtr* dest_t = _igvn.type(dest)->is_oopptr(); - if (dest_t->is_known_instance() && !is_partial_array_copy) { + if (dest_t->is_known_instance()) { ArrayCopyNode* ac = nullptr; assert(ArrayCopyNode::may_modify(dest_t, (*ctrl)->in(0)->as_MemBar(), &_igvn, ac), "dependency on arraycopy lost"); assert(ac == nullptr, "no arraycopy anymore"); @@ -1174,14 +1187,16 @@ Node* PhaseMacroExpand::generate_generic_arraycopy(Node** ctrl, MergeMemNode** m } // Helper function; generates the fast out-of-line call to an arraycopy stub. -bool PhaseMacroExpand::generate_unchecked_arraycopy(Node** ctrl, MergeMemNode** mem, +void PhaseMacroExpand::generate_unchecked_arraycopy(Node** ctrl, MergeMemNode** mem, const TypePtr* adr_type, BasicType basic_elem_type, bool disjoint_bases, Node* src, Node* src_offset, Node* dest, Node* dest_offset, Node* copy_length, bool dest_uninitialized) { - if ((*ctrl)->is_top()) return false; + if ((*ctrl)->is_top()) { + return; + } Node* src_start = src; Node* dest_start = dest; @@ -1226,9 +1241,7 @@ bool PhaseMacroExpand::generate_unchecked_arraycopy(Node** ctrl, MergeMemNode** } transform_later(*mem); *ctrl = exit_block; - return true; } - return false; } #undef XTOP @@ -1311,7 +1324,7 @@ void PhaseMacroExpand::expand_arraycopy_node(ArrayCopyNode *ac) { // Do not let writes into the source float below the arraycopy. { Node* mem = ac->in(TypeFunc::Memory); - insert_mem_bar(&ctrl, &mem, Op_MemBarCPUOrder); + insert_mem_bar(&ctrl, &mem, Op_MemBarCPUOrder, Compile::AliasIdxBot); merge_mem = MergeMemNode::make(mem); transform_later(merge_mem); diff --git a/src/hotspot/share/opto/memnode.hpp b/src/hotspot/share/opto/memnode.hpp index eca891bebbb..f17db05e683 100644 --- a/src/hotspot/share/opto/memnode.hpp +++ b/src/hotspot/share/opto/memnode.hpp @@ -1132,7 +1132,7 @@ class MemBarNode: public MultiNode { LeadingStore, TrailingLoadStore, LeadingLoadStore, - TrailingPartialArrayCopy + TrailingExpandedArrayCopy } _kind; #ifdef ASSERT @@ -1169,8 +1169,8 @@ public: bool trailing() const { return _kind == TrailingLoad || _kind == TrailingStore || _kind == TrailingLoadStore; } bool leading() const { return _kind == LeadingStore || _kind == LeadingLoadStore; } bool standalone() const { return _kind == Standalone; } - void set_trailing_partial_array_copy() { _kind = TrailingPartialArrayCopy; } - bool trailing_partial_array_copy() const { return _kind == TrailingPartialArrayCopy; } + void set_trailing_expanded_array_copy() { _kind = TrailingExpandedArrayCopy; } + bool trailing_expanded_array_copy() const { return _kind == TrailingExpandedArrayCopy; } static void set_store_pair(MemBarNode* leading, MemBarNode* trailing); static void set_load_store_pair(MemBarNode* leading, MemBarNode* trailing); diff --git a/test/hotspot/jtreg/compiler/arraycopy/TestSunkLoadAntiDependency.java b/test/hotspot/jtreg/compiler/arraycopy/TestSunkLoadAntiDependency.java new file mode 100644 index 00000000000..2cb185f58bf --- /dev/null +++ b/test/hotspot/jtreg/compiler/arraycopy/TestSunkLoadAntiDependency.java @@ -0,0 +1,66 @@ +/* + * 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 8341976 + * @summary C2: use_mem_state != load->find_exact_control(load->in(0)) assert failure + * @run main/othervm -XX:-BackgroundCompilation TestSunkLoadAntiDependency + * @run main TestSunkLoadAntiDependency + */ + +public class TestSunkLoadAntiDependency { + private static volatile int volatileField; + + public static void main(String[] args) { + int[] array = new int[100]; + for (int i = 0; i < 20_000; i++) { + test1(array, 2); + inlined1(array, 100, 0, 100, array); + } + } + + private static int test1(int[] src, int length) { + length = Integer.max(1, length); + int[] dst = new int[2]; + int stop; + for (stop = 0; stop < 10; stop++) { + for (int i = 0; i < 10; i++) { + } + } + int start; + for (start = 0; start < 9; start++) { + for (int i = 0; i < 10; i++) { + } + } + inlined1(src, length, start, stop, dst); + return dst[0] + dst[1]; + } + + private static void inlined1(int[] src, int length, int start, int stop, int[] dst) { + for (int i = start; i < stop; i++) { + volatileField = 42; + System.arraycopy(src, 0, dst, 0, length); + } + } +}