8324969: C2: prevent elimination of unbalanced coarsened locking regions

Reviewed-by: epeter, vlivanov, dlong

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

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<AbstractLockNode*>& 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
  */

src/hotspot/share/opto/compile.hpp

@@ -780,6 +780,7 @@ private:
   void add_coarsened_locks(GrowableArray<AbstractLockNode*>& 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;  }

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<Node*>& 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
 

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);
 

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++) {

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; }
+  bool is_regular()    const { return _kind == Regular; }
-  // mark lock as eliminated.
+  bool is_local()      const { return _kind == Local; }
-  void set_eliminated()      { _is_eliminated = true; }
+  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);

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) {
+void PhaseMacroExpand::mark_eliminated_box(Node* box, Node* obj) {
-  if (oldbox->as_BoxLock()->is_eliminated()) {
+  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

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;
+    }
+}