diff --git a/src/hotspot/share/opto/callnode.cpp b/src/hotspot/share/opto/callnode.cpp index 3379a9b01e0..98837e5e046 100644 --- a/src/hotspot/share/opto/callnode.cpp +++ b/src/hotspot/share/opto/callnode.cpp @@ -2002,7 +2002,7 @@ Node *LockNode::Ideal(PhaseGVN *phase, bool can_reshape) { // If we are locking an non-escaped object, the lock/unlock is unnecessary // ConnectionGraph *cgr = phase->C->congraph(); - if (cgr != nullptr && cgr->not_global_escape(obj_node())) { + if (cgr != nullptr && cgr->can_eliminate_lock(this)) { assert(!is_eliminated() || is_coarsened(), "sanity"); // The lock could be marked eliminated by lock coarsening // code during first IGVN before EA. Replace coarsened flag @@ -2165,6 +2165,7 @@ bool LockNode::is_nested_lock_region(Compile * c) { obj_node = bs->step_over_gc_barrier(obj_node); BoxLockNode* box_node = sfn->monitor_box(jvms, idx)->as_BoxLock(); if ((box_node->stack_slot() < stk_slot) && obj_node->eqv_uncast(obj)) { + box->set_nested(); return true; } } @@ -2198,7 +2199,7 @@ Node *UnlockNode::Ideal(PhaseGVN *phase, bool can_reshape) { // If we are unlocking an non-escaped object, the lock/unlock is unnecessary. // ConnectionGraph *cgr = phase->C->congraph(); - if (cgr != nullptr && cgr->not_global_escape(obj_node())) { + if (cgr != nullptr && cgr->can_eliminate_lock(this)) { assert(!is_eliminated() || is_coarsened(), "sanity"); // The lock could be marked eliminated by lock coarsening // code during first IGVN before EA. Replace coarsened flag diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp index 83b922492b3..1e4c8fc51b7 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -56,6 +56,7 @@ #include "opto/divnode.hpp" #include "opto/escape.hpp" #include "opto/idealGraphPrinter.hpp" +#include "opto/locknode.hpp" #include "opto/loopnode.hpp" #include "opto/machnode.hpp" #include "opto/macro.hpp" @@ -4867,10 +4868,25 @@ void Compile::add_coarsened_locks(GrowableArray& locks) { if (length > 0) { // Have to keep this list until locks elimination during Macro nodes elimination. Lock_List* locks_list = new (comp_arena()) Lock_List(comp_arena(), length); + AbstractLockNode* alock = locks.at(0); + BoxLockNode* box = alock->box_node()->as_BoxLock(); for (int i = 0; i < length; i++) { AbstractLockNode* lock = locks.at(i); assert(lock->is_coarsened(), "expecting only coarsened AbstractLock nodes, but got '%s'[%d] node", lock->Name(), lock->_idx); locks_list->push(lock); + BoxLockNode* this_box = lock->box_node()->as_BoxLock(); + if (this_box != box) { + // Locking regions (BoxLock) could be Unbalanced here: + // - its coarsened locks were eliminated in earlier + // macro nodes elimination followed by loop unroll + // Preserve Unbalanced status in such cases. + if (!this_box->is_unbalanced()) { + this_box->set_coarsened(); + } + if (!box->is_unbalanced()) { + box->set_coarsened(); + } + } } _coarsened_locks.append(locks_list); } @@ -4948,6 +4964,38 @@ bool Compile::coarsened_locks_consistent() { return true; } +// Mark locking regions (identified by BoxLockNode) as unbalanced if +// locks coarsening optimization removed Lock/Unlock nodes from them. +// Such regions become unbalanced because coarsening only removes part +// of Lock/Unlock nodes in region. As result we can't execute other +// locks elimination optimizations which assume all code paths have +// corresponding pair of Lock/Unlock nodes - they are balanced. +void Compile::mark_unbalanced_boxes() const { + int count = coarsened_count(); + for (int i = 0; i < count; i++) { + Node_List* locks_list = _coarsened_locks.at(i); + uint size = locks_list->size(); + if (size > 0) { + AbstractLockNode* alock = locks_list->at(0)->as_AbstractLock(); + BoxLockNode* box = alock->box_node()->as_BoxLock(); + if (alock->is_coarsened()) { + // coarsened_locks_consistent(), which is called before this method, verifies + // that the rest of Lock/Unlock nodes on locks_list are also coarsened. + assert(!box->is_eliminated(), "regions with coarsened locks should not be marked as eliminated"); + for (uint j = 1; j < size; j++) { + assert(locks_list->at(j)->as_AbstractLock()->is_coarsened(), "only coarsened locks are expected here"); + BoxLockNode* this_box = locks_list->at(j)->as_AbstractLock()->box_node()->as_BoxLock(); + if (box != this_box) { + assert(!this_box->is_eliminated(), "regions with coarsened locks should not be marked as eliminated"); + box->set_unbalanced(); + this_box->set_unbalanced(); + } + } + } + } + } +} + /** * Remove the speculative part of types and clean up the graph */ diff --git a/src/hotspot/share/opto/compile.hpp b/src/hotspot/share/opto/compile.hpp index a5e4b69d263..917ceb52d9a 100644 --- a/src/hotspot/share/opto/compile.hpp +++ b/src/hotspot/share/opto/compile.hpp @@ -780,6 +780,7 @@ private: void add_coarsened_locks(GrowableArray& locks); void remove_coarsened_lock(Node* n); bool coarsened_locks_consistent(); + void mark_unbalanced_boxes() const; bool post_loop_opts_phase() { return _post_loop_opts_phase; } void set_post_loop_opts_phase() { _post_loop_opts_phase = true; } diff --git a/src/hotspot/share/opto/escape.cpp b/src/hotspot/share/opto/escape.cpp index ec9c0b79fb0..99a96a45be6 100644 --- a/src/hotspot/share/opto/escape.cpp +++ b/src/hotspot/share/opto/escape.cpp @@ -37,6 +37,7 @@ #include "opto/compile.hpp" #include "opto/escape.hpp" #include "opto/macro.hpp" +#include "opto/locknode.hpp" #include "opto/phaseX.hpp" #include "opto/movenode.hpp" #include "opto/rootnode.hpp" @@ -2547,7 +2548,7 @@ void ConnectionGraph::optimize_ideal_graph(GrowableArray& ptr_cmp_worklis if (n->is_AbstractLock()) { // Lock and Unlock nodes AbstractLockNode* alock = n->as_AbstractLock(); if (!alock->is_non_esc_obj()) { - if (not_global_escape(alock->obj_node())) { + if (can_eliminate_lock(alock)) { assert(!alock->is_eliminated() || alock->is_coarsened(), "sanity"); // The lock could be marked eliminated by lock coarsening // code during first IGVN before EA. Replace coarsened flag @@ -2880,6 +2881,21 @@ bool ConnectionGraph::not_global_escape(Node *n) { return true; } +// Return true if locked object does not escape globally +// and locked code region (identified by BoxLockNode) is balanced: +// all compiled code paths have corresponding Lock/Unlock pairs. +bool ConnectionGraph::can_eliminate_lock(AbstractLockNode* alock) { + BoxLockNode* box = alock->box_node()->as_BoxLock(); + if (!box->is_unbalanced() && not_global_escape(alock->obj_node())) { + if (EliminateNestedLocks) { + // We can mark whole locking region as Local only when only + // one object is used for locking. + box->set_local(); + } + return true; + } + return false; +} // Helper functions diff --git a/src/hotspot/share/opto/escape.hpp b/src/hotspot/share/opto/escape.hpp index 7e6bd87d493..4ed7fb7f054 100644 --- a/src/hotspot/share/opto/escape.hpp +++ b/src/hotspot/share/opto/escape.hpp @@ -112,6 +112,7 @@ class Compile; class Node; +class AbstractLockNode; class CallNode; class PhiNode; class PhaseTransform; @@ -630,6 +631,8 @@ public: bool not_global_escape(Node *n); + bool can_eliminate_lock(AbstractLockNode* alock); + // To be used by, e.g., BarrierSetC2 impls Node* get_addp_base(Node* addp); diff --git a/src/hotspot/share/opto/locknode.cpp b/src/hotspot/share/opto/locknode.cpp index d73150684a7..17d26d620e5 100644 --- a/src/hotspot/share/opto/locknode.cpp +++ b/src/hotspot/share/opto/locknode.cpp @@ -40,7 +40,7 @@ const RegMask &BoxLockNode::out_RegMask() const { uint BoxLockNode::size_of() const { return sizeof(*this); } BoxLockNode::BoxLockNode( int slot ) : Node( Compile::current()->root() ), - _slot(slot), _is_eliminated(false) { + _slot(slot), _kind(BoxLockNode::Regular) { init_class_id(Class_BoxLock); init_flags(Flag_rematerialize); OptoReg::Name reg = OptoReg::stack2reg(_slot); @@ -51,19 +51,48 @@ BoxLockNode::BoxLockNode( int slot ) : Node( Compile::current()->root() ), _inmask.Insert(reg); } -//-----------------------------hash-------------------------------------------- uint BoxLockNode::hash() const { - if (EliminateNestedLocks) + if (EliminateNestedLocks) { return NO_HASH; // Each locked region has own BoxLock node - return Node::hash() + _slot + (_is_eliminated ? Compile::current()->fixed_slots() : 0); + } + return Node::hash() + _slot + (is_eliminated() ? Compile::current()->fixed_slots() : 0); } -//------------------------------cmp-------------------------------------------- bool BoxLockNode::cmp( const Node &n ) const { - if (EliminateNestedLocks) + if (EliminateNestedLocks) { return (&n == this); // Always fail except on self + } const BoxLockNode &bn = (const BoxLockNode &)n; - return bn._slot == _slot && bn._is_eliminated == _is_eliminated; + return (bn._slot == _slot) && (bn.is_eliminated() == is_eliminated()); +} + +Node* BoxLockNode::Identity(PhaseGVN* phase) { + if (!EliminateNestedLocks && !this->is_eliminated()) { + Node* n = phase->hash_find(this); + if (n == nullptr || n == this) { + return this; + } + BoxLockNode* old_box = n->as_BoxLock(); + // Set corresponding status (_kind) when commoning BoxLock nodes. + if (this->_kind != old_box->_kind) { + if (this->is_unbalanced()) { + old_box->set_unbalanced(); + } + if (!old_box->is_unbalanced()) { + // Only Regular or Coarsened status should be here: + // Nested and Local are set only when EliminateNestedLocks is on. + if (old_box->is_regular()) { + assert(this->is_coarsened(),"unexpected kind: %s", _kind_name[(int)this->_kind]); + old_box->set_coarsened(); + } else { + assert(this->is_regular(),"unexpected kind: %s", _kind_name[(int)this->_kind]); + assert(old_box->is_coarsened(),"unexpected kind: %s", _kind_name[(int)old_box->_kind]); + } + } + } + return old_box; + } + return this; } BoxLockNode* BoxLockNode::box_node(Node* box) { @@ -90,6 +119,9 @@ OptoReg::Name BoxLockNode::reg(Node* box) { // Is BoxLock node used for one simple lock region (same box and obj)? bool BoxLockNode::is_simple_lock_region(LockNode** unique_lock, Node* obj, Node** bad_lock) { + if (is_unbalanced()) { + return false; + } LockNode* lock = nullptr; bool has_one_lock = false; for (uint i = 0; i < this->outcnt(); i++) { diff --git a/src/hotspot/share/opto/locknode.hpp b/src/hotspot/share/opto/locknode.hpp index 4a74e50425f..3bc684c40a9 100644 --- a/src/hotspot/share/opto/locknode.hpp +++ b/src/hotspot/share/opto/locknode.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2024, Oracle and/or its affiliates. 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 @@ -33,9 +33,37 @@ class RTMLockingCounters; //------------------------------BoxLockNode------------------------------------ class BoxLockNode : public Node { +private: const int _slot; // stack slot RegMask _inmask; // OptoReg corresponding to stack slot - bool _is_eliminated; // Associated locks were safely eliminated + enum { + Regular = 0, // Normal locking region + Local, // EA found that local not escaping object is used for locking + Nested, // This region is inside other region which use the same object + Coarsened, // Some lock/unlock in region were marked as coarsened + Unbalanced, // This region become unbalanced after coarsened lock/unlock were eliminated + // or it is locking region from OSR when locking is done in Interpreter + Eliminated // All lock/unlock in region were eliminated + } _kind; + +#ifdef ASSERT + const char* _kind_name[6] = { + "Regular", + "Local", + "Nested", + "Coarsened", + "Unbalanced", + "Eliminated" + }; +#endif + + // Allowed transitions of _kind: + // Regular -> Local, Nested, Coarsened + // Local -> Eliminated + // Nested -> Eliminated + // Coarsened -> Local, Nested, Unbalanced + // EA and nested lock elimination can overwrite Coarsened kind. + // Also allow transition to the same kind. public: BoxLockNode( int lock ); @@ -49,6 +77,7 @@ public: virtual bool cmp( const Node &n ) const; virtual const class Type *bottom_type() const { return TypeRawPtr::BOTTOM; } virtual uint ideal_reg() const { return Op_RegP; } + virtual Node* Identity(PhaseGVN* phase); static OptoReg::Name reg(Node* box_node); static BoxLockNode* box_node(Node* box_node); @@ -57,9 +86,38 @@ public: } int stack_slot() const { return _slot; } - bool is_eliminated() const { return _is_eliminated; } - // mark lock as eliminated. - void set_eliminated() { _is_eliminated = true; } + bool is_regular() const { return _kind == Regular; } + bool is_local() const { return _kind == Local; } + bool is_nested() const { return _kind == Nested; } + bool is_coarsened() const { return _kind == Coarsened; } + bool is_eliminated() const { return _kind == Eliminated; } + bool is_unbalanced() const { return _kind == Unbalanced; } + + void set_local() { + assert((_kind == Regular || _kind == Local || _kind == Coarsened), + "incorrect kind for Local transitioni: %s", _kind_name[(int)_kind]); + _kind = Local; + } + void set_nested() { + assert((_kind == Regular || _kind == Nested || _kind == Coarsened), + "incorrect kind for Nested transition: %s", _kind_name[(int)_kind]); + _kind = Nested; + } + void set_coarsened() { + assert((_kind == Regular || _kind == Coarsened), + "incorrect kind for Coarsened transition: %s", _kind_name[(int)_kind]); + _kind = Coarsened; + } + void set_eliminated() { + assert((_kind == Local || _kind == Nested), + "incorrect kind for Eliminated transition: %s", _kind_name[(int)_kind]); + _kind = Eliminated; + } + void set_unbalanced() { + assert((_kind == Coarsened || _kind == Unbalanced), + "incorrect kind for Unbalanced transition: %s", _kind_name[(int)_kind]); + _kind = Unbalanced; + } // Is BoxLock node used for one simple lock region? bool is_simple_lock_region(LockNode** unique_lock, Node* obj, Node** bad_lock); diff --git a/src/hotspot/share/opto/macro.cpp b/src/hotspot/share/opto/macro.cpp index c2aa017197e..23928b554e4 100644 --- a/src/hotspot/share/opto/macro.cpp +++ b/src/hotspot/share/opto/macro.cpp @@ -1942,10 +1942,12 @@ void PhaseMacroExpand::expand_allocate_array(AllocateArrayNode *alloc) { // marked for elimination since new obj has no escape information. // Mark all associated (same box and obj) lock and unlock nodes for // elimination if some of them marked already. -void PhaseMacroExpand::mark_eliminated_box(Node* oldbox, Node* obj) { - if (oldbox->as_BoxLock()->is_eliminated()) { +void PhaseMacroExpand::mark_eliminated_box(Node* box, Node* obj) { + BoxLockNode* oldbox = box->as_BoxLock(); + if (oldbox->is_eliminated()) { return; // This BoxLock node was processed already. } + assert(!oldbox->is_unbalanced(), "this should not be called for unbalanced region"); // New implementation (EliminateNestedLocks) has separate BoxLock // node for each locked region so mark all associated locks/unlocks as // eliminated even if different objects are referenced in one locked region @@ -1953,8 +1955,9 @@ void PhaseMacroExpand::mark_eliminated_box(Node* oldbox, Node* obj) { if (EliminateNestedLocks || oldbox->as_BoxLock()->is_simple_lock_region(nullptr, obj, nullptr)) { // Box is used only in one lock region. Mark this box as eliminated. + oldbox->set_local(); // This verifies correct state of BoxLock _igvn.hash_delete(oldbox); - oldbox->as_BoxLock()->set_eliminated(); // This changes box's hash value + oldbox->set_eliminated(); // This changes box's hash value _igvn.hash_insert(oldbox); for (uint i = 0; i < oldbox->outcnt(); i++) { @@ -1981,6 +1984,7 @@ void PhaseMacroExpand::mark_eliminated_box(Node* oldbox, Node* obj) { // Note: BoxLock node is marked eliminated only here and it is used // to indicate that all associated lock and unlock nodes are marked // for elimination. + newbox->set_local(); // This verifies correct state of BoxLock newbox->set_eliminated(); transform_later(newbox); @@ -2036,6 +2040,9 @@ void PhaseMacroExpand::mark_eliminated_box(Node* oldbox, Node* obj) { //-----------------------mark_eliminated_locking_nodes----------------------- void PhaseMacroExpand::mark_eliminated_locking_nodes(AbstractLockNode *alock) { + if (alock->box_node()->as_BoxLock()->is_unbalanced()) { + return; // Can't do any more elimination for this locking region + } if (EliminateNestedLocks) { if (alock->is_nested()) { assert(alock->box_node()->as_BoxLock()->is_eliminated(), "sanity"); @@ -2352,6 +2359,11 @@ void PhaseMacroExpand::eliminate_macro_nodes() { // Re-marking may break consistency of Coarsened locks. if (!C->coarsened_locks_consistent()) { return; // recompile without Coarsened locks if broken + } else { + // After coarsened locks are eliminated locking regions + // become unbalanced. We should not execute any more + // locks elimination optimizations on them. + C->mark_unbalanced_boxes(); } // First, attempt to eliminate locks diff --git a/test/hotspot/jtreg/compiler/locks/TestCoarsenedAndNestedLocksElimination.java b/test/hotspot/jtreg/compiler/locks/TestCoarsenedAndNestedLocksElimination.java new file mode 100644 index 00000000000..d1b7a2eda9b --- /dev/null +++ b/test/hotspot/jtreg/compiler/locks/TestCoarsenedAndNestedLocksElimination.java @@ -0,0 +1,82 @@ +/* + * Copyright (c) 2024, Oracle and/or its affiliates. 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 8324969 + * @summary C2 incorrectly marks unbalanced (after coarsened locks were eliminated) + * nested locks for elimination. + * @requires vm.compMode != "Xint" + * @run main/othervm -XX:-BackgroundCompilation TestCoarsenedAndNestedLocksElimination + */ + +public class TestCoarsenedAndNestedLocksElimination { + + public static void main(String[] strArr) { + for (int i = 0; i < 12000; ++i) { + test1(-1); + test2(-1); + } + } + + static synchronized int methodA(int var) { + return var; + } + + static synchronized int methodB(int var) { + return var; + } + + static int varA = 0; + static int varB = 0; + + static void test1(int var) { + synchronized (TestNestedLocksElimination.class) { + for (int i2 = 0; i2 < 3; i2++) { // Fully unrolled + varA = methodA(i2); // Nested synchronized methods also use + varB = i2 + methodB(var); // TestNestedLocksElimination.class for lock + } + } + TestNestedLocksElimination t = new TestNestedLocksElimination(); // Triggers EA + } + + static boolean test2(int var) { + synchronized (TestNestedLocksElimination.class) { + for (int i1 = 0; i1 < 100; i1++) { + switch (42) { + case 42: + short[] sArr = new short[256]; // Big enough to avoid scalarization checks + case 50: + for (int i2 = 2; i2 < 8; i2 += 2) { // Fully unrolled + for (int i3 = 1;;) { + int var1 = methodA(i2); + int var2 = i2 + methodB(i3); + break; + } + } + } + } + } + return var > 0; + } +}