8227407: ZGC: C2 loads and load barriers can get separated by safepoints
authoreosterlund
Mon, 29 Jul 2019 13:57:54 +0200
changeset 57582 a79a819a8218
parent 57581 57a391a23f7f
child 57583 aad50831e169
8227407: ZGC: C2 loads and load barriers can get separated by safepoints Reviewed-by: neliasso, smonteith, roland
src/hotspot/share/gc/shared/c2/barrierSetC2.cpp
src/hotspot/share/gc/shared/c2/barrierSetC2.hpp
src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp
src/hotspot/share/gc/z/c2/zBarrierSetC2.hpp
src/hotspot/share/opto/compile.cpp
src/hotspot/share/opto/macro.cpp
src/hotspot/share/opto/memnode.cpp
src/hotspot/share/opto/memnode.hpp
src/hotspot/share/opto/superword.cpp
--- a/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp	Mon Jul 29 13:57:54 2019 +0200
@@ -132,13 +132,13 @@
   bool requires_atomic_access = (decorators & MO_UNORDERED) == 0;
   bool unaligned = (decorators & C2_UNALIGNED) != 0;
   bool control_dependent = (decorators & C2_CONTROL_DEPENDENT_LOAD) != 0;
-  bool pinned = (decorators & C2_PINNED_LOAD) != 0;
+  bool unknown_control = (decorators & C2_UNKNOWN_CONTROL_LOAD) != 0;
   bool unsafe = (decorators & C2_UNSAFE_ACCESS) != 0;
 
   bool in_native = (decorators & IN_NATIVE) != 0;
 
   MemNode::MemOrd mo = access.mem_node_mo();
-  LoadNode::ControlDependency dep = pinned ? LoadNode::Pinned : LoadNode::DependsOnlyOnTest;
+  LoadNode::ControlDependency dep = unknown_control ? LoadNode::UnknownControl : LoadNode::DependsOnlyOnTest;
 
   Node* load;
   if (access.is_parse_access()) {
@@ -349,7 +349,7 @@
     // To be valid, unsafe loads may depend on other conditions than
     // the one that guards them: pin the Load node
     _decorators |= C2_CONTROL_DEPENDENT_LOAD;
-    _decorators |= C2_PINNED_LOAD;
+    _decorators |= C2_UNKNOWN_CONTROL_LOAD;
     const TypePtr* adr_type = _addr.type();
     Node* adr = _addr.node();
     if (!needs_cpu_membar() && adr_type->isa_instptr()) {
@@ -361,7 +361,7 @@
         if (offset < s) {
           // Guaranteed to be a valid access, no need to pin it
           _decorators ^= C2_CONTROL_DEPENDENT_LOAD;
-          _decorators ^= C2_PINNED_LOAD;
+          _decorators ^= C2_UNKNOWN_CONTROL_LOAD;
         }
       }
     }
--- a/src/hotspot/share/gc/shared/c2/barrierSetC2.hpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/gc/shared/c2/barrierSetC2.hpp	Mon Jul 29 13:57:54 2019 +0200
@@ -42,8 +42,8 @@
 const DecoratorSet C2_WEAK_CMPXCHG           = DECORATOR_LAST << 3;
 // This denotes that a load has control dependency.
 const DecoratorSet C2_CONTROL_DEPENDENT_LOAD = DECORATOR_LAST << 4;
-// This denotes that a load that must be pinned.
-const DecoratorSet C2_PINNED_LOAD            = DECORATOR_LAST << 5;
+// This denotes that a load that must be pinned, but may float above safepoints.
+const DecoratorSet C2_UNKNOWN_CONTROL_LOAD   = DECORATOR_LAST << 5;
 // This denotes that the access is produced from the sun.misc.Unsafe intrinsics.
 const DecoratorSet C2_UNSAFE_ACCESS          = DECORATOR_LAST << 6;
 // This denotes that the access mutates state.
--- a/src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp	Mon Jul 29 13:57:54 2019 +0200
@@ -557,7 +557,6 @@
   result_phi->set_req(1, new_loadp);
   result_phi->set_req(2, barrier->in(LoadBarrierNode::Oop));
 
-
   igvn.replace_node(out_ctrl, result_region);
   igvn.replace_node(out_res, result_phi);
 
@@ -740,29 +739,61 @@
 
 #ifdef ASSERT
 
-static bool look_for_barrier(Node* n, bool post_parse, VectorSet& visited) {
-  if (visited.test_set(n->_idx)) {
-    return true;
-  }
+static void verify_slippery_safepoints_internal(Node* ctrl) {
+  // Given a CFG node, make sure it does not contain both safepoints and loads
+  // that have expanded barriers.
+  bool found_safepoint = false;
+  bool found_load = false;
 
-  for (DUIterator_Fast imax, i = n->fast_outs(imax); i < imax; i++) {
-    Node* u = n->fast_out(i);
-    if (u->is_LoadBarrier()) {
-    } else if ((u->is_Phi() || u->is_CMove()) && !post_parse) {
-      if (!look_for_barrier(u, post_parse, visited)) {
-        return false;
-      }
-    } else if (u->Opcode() == Op_EncodeP || u->Opcode() == Op_DecodeN) {
-      if (!look_for_barrier(u, post_parse, visited)) {
-        return false;
-      }
-    } else if (u->Opcode() != Op_SCMemProj) {
-      tty->print("bad use"); u->dump();
-      return false;
+  for (DUIterator_Fast imax, i = ctrl->fast_outs(imax); i < imax; i++) {
+    Node* node = ctrl->fast_out(i);
+    if (node->in(0) != ctrl) {
+      // Skip outgoing precedence edges from ctrl.
+      continue;
+    }
+    if (node->is_SafePoint()) {
+      found_safepoint = true;
+    }
+    if (node->is_Load() && load_require_barrier(node->as_Load()) &&
+        load_has_expanded_barrier(node->as_Load())) {
+      found_load = true;
     }
   }
+  assert(!found_safepoint || !found_load, "found load and safepoint in same block");
+}
 
-  return true;
+static void verify_slippery_safepoints(Compile* C) {
+  ResourceArea *area = Thread::current()->resource_area();
+  Unique_Node_List visited(area);
+  Unique_Node_List checked(area);
+
+  // Recursively walk the graph.
+  visited.push(C->root());
+  while (visited.size() > 0) {
+    Node* node = visited.pop();
+
+    Node* ctrl = node;
+    if (!node->is_CFG()) {
+      ctrl = node->in(0);
+    }
+
+    if (ctrl != NULL && !checked.member(ctrl)) {
+      // For each block found in the graph, verify that it does not
+      // contain both a safepoint and a load requiring barriers.
+      verify_slippery_safepoints_internal(ctrl);
+
+      checked.push(ctrl);
+    }
+
+    checked.push(node);
+
+    for (DUIterator_Fast imax, i = node->fast_outs(imax); i < imax; i++) {
+      Node* use = node->fast_out(i);
+      if (checked.member(use))  continue;
+      if (visited.member(use))  continue;
+      visited.push(use);
+    }
+  }
 }
 
 void ZBarrierSetC2::verify_gc_barriers(Compile* compile, CompilePhase phase) const {
@@ -778,6 +809,7 @@
     case BarrierSetC2::BeforeCodeGen:
       // Barriers has been fully expanded.
       assert(state()->load_barrier_count() == 0, "No more macro barriers");
+      verify_slippery_safepoints(compile);
       break;
     default:
       assert(0, "Phase without verification");
@@ -1594,7 +1626,7 @@
   // For all successors of ctrl - move all visited to become successors of barrier_ctrl instead
   for (DUIterator_Fast imax, r = ctrl->fast_outs(imax); r < imax; r++) {
     Node* tmp = ctrl->fast_out(r);
-    if (visited2.test(tmp->_idx) && (tmp != load)) {
+    if (tmp->is_SafePoint() || (visited2.test(tmp->_idx) && (tmp != load))) {
       if (trace) tty->print_cr(" Move ctrl_succ %i to barrier_ctrl", tmp->_idx);
       igvn.replace_input_of(tmp, 0, barrier_ctrl);
       --r; --imax;
@@ -1622,6 +1654,9 @@
   barrier->set_req(LoadBarrierNode::Oop, load);
   barrier->set_req(LoadBarrierNode::Control, ctrl);
 
+  igvn.replace_input_of(load, MemNode::Control, ctrl);
+  load->pin();
+
   igvn.rehash_node_delayed(load);
   igvn.register_new_node_with_optimizer(barrier);
   igvn.register_new_node_with_optimizer(barrier_val);
--- a/src/hotspot/share/gc/z/c2/zBarrierSetC2.hpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/gc/z/c2/zBarrierSetC2.hpp	Mon Jul 29 13:57:54 2019 +0200
@@ -202,6 +202,7 @@
   virtual void barrier_insertion_phase(Compile* C, PhaseIterGVN &igvn) const;
   virtual bool optimize_loops(PhaseIdealLoop* phase, LoopOptsMode mode, VectorSet& visited, Node_Stack& nstack, Node_List& worklist) const;
   virtual bool is_gc_specific_loop_opts_pass(LoopOptsMode mode) const { return (mode == LoopOptsZBarrierInsertion); }
+  virtual bool strip_mined_loops_expanded(LoopOptsMode mode) const { return mode == LoopOptsZBarrierInsertion; }
 
 private:
   // Load barrier insertion and expansion internal
--- a/src/hotspot/share/opto/compile.cpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/opto/compile.cpp	Mon Jul 29 13:57:54 2019 +0200
@@ -2400,13 +2400,6 @@
   }
 
 #ifdef ASSERT
-  bs->verify_gc_barriers(this, BarrierSetC2::BeforeLateInsertion);
-#endif
-
-  bs->barrier_insertion_phase(C, igvn);
-  if (failing())  return;
-
-#ifdef ASSERT
   bs->verify_gc_barriers(this, BarrierSetC2::BeforeMacroExpand);
 #endif
 
@@ -2420,6 +2413,13 @@
     print_method(PHASE_MACRO_EXPANSION, 2);
   }
 
+#ifdef ASSERT
+  bs->verify_gc_barriers(this, BarrierSetC2::BeforeLateInsertion);
+#endif
+
+  bs->barrier_insertion_phase(C, igvn);
+  if (failing())  return;
+
   {
     TracePhase tp("barrierExpand", &timers[_t_barrierExpand]);
     if (bs->expand_barriers(this, igvn)) {
--- a/src/hotspot/share/opto/macro.cpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/opto/macro.cpp	Mon Jul 29 13:57:54 2019 +0200
@@ -351,7 +351,7 @@
     Node* base = ac->in(ArrayCopyNode::Src)->in(AddPNode::Base);
     Node* adr = _igvn.transform(new AddPNode(base, base, MakeConX(offset)));
     const TypePtr* adr_type = _igvn.type(base)->is_ptr()->add_offset(offset);
-    res = LoadNode::make(_igvn, ctl, mem, adr, adr_type, type, bt, MemNode::unordered, LoadNode::Pinned);
+    res = LoadNode::make(_igvn, ctl, mem, adr, adr_type, type, bt, MemNode::unordered, LoadNode::UnknownControl);
   } else {
     if (ac->modifies(offset, offset, &_igvn, true)) {
       assert(ac->in(ArrayCopyNode::Dest) == alloc->result_cast(), "arraycopy destination should be allocation's result");
@@ -366,7 +366,7 @@
       Node* base = ac->in(ArrayCopyNode::Src);
       Node* adr = _igvn.transform(new AddPNode(base, base, off));
       const TypePtr* adr_type = _igvn.type(base)->is_ptr()->add_offset(offset);
-      res = LoadNode::make(_igvn, ctl, mem, adr, adr_type, type, bt, MemNode::unordered, LoadNode::Pinned);
+      res = LoadNode::make(_igvn, ctl, mem, adr, adr_type, type, bt, MemNode::unordered, LoadNode::UnknownControl);
     }
   }
   if (res != NULL) {
--- a/src/hotspot/share/opto/memnode.cpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/opto/memnode.cpp	Mon Jul 29 13:57:54 2019 +0200
@@ -984,7 +984,7 @@
     ld->set_req(0, ctl);
     ld->set_req(MemNode::Memory, mem);
     // load depends on the tests that validate the arraycopy
-    ld->_control_dependency = Pinned;
+    ld->_control_dependency = UnknownControl;
     return ld;
   }
   return NULL;
@@ -1577,7 +1577,8 @@
   // pointer stores & cardmarks must stay on the same side of a SafePoint.
   if( ctrl != NULL && ctrl->Opcode() == Op_SafePoint &&
       phase->C->get_alias_index(phase->type(address)->is_ptr()) != Compile::AliasIdxRaw  &&
-      !addr_mark ) {
+      !addr_mark &&
+      (depends_only_on_test() || has_unknown_control_dependency())) {
     ctrl = ctrl->in(0);
     set_req(MemNode::Control,ctrl);
     progress = true;
--- a/src/hotspot/share/opto/memnode.hpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/opto/memnode.hpp	Mon Jul 29 13:57:54 2019 +0200
@@ -154,14 +154,12 @@
   // Some loads (from unsafe) should be pinned: they don't depend only
   // on the dominating test.  The field _control_dependency below records
   // whether that node depends only on the dominating test.
-  // Methods used to build LoadNodes pass an argument of type enum
-  // ControlDependency instead of a boolean because those methods
-  // typically have multiple boolean parameters with default values:
-  // passing the wrong boolean to one of these parameters by mistake
-  // goes easily unnoticed. Using an enum, the compiler can check that
-  // the type of a value and the type of the parameter match.
+  // Pinned and UnknownControl are similar, but differ in that Pinned
+  // loads are not allowed to float across safepoints, whereas UnknownControl
+  // loads are allowed to do that. Therefore, Pinned is stricter.
   enum ControlDependency {
     Pinned,
+    UnknownControl,
     DependsOnlyOnTest
   };
 
@@ -269,6 +267,9 @@
   uint barrier_data() { return _barrier; }
   void set_barrier_data(uint barrier_data) { _barrier |= barrier_data; }
 
+  void pin() { _control_dependency = Pinned; }
+  bool has_unknown_control_dependency() const { return _control_dependency == UnknownControl; }
+
 #ifndef PRODUCT
   virtual void dump_spec(outputStream *st) const;
 #endif
--- a/src/hotspot/share/opto/superword.cpp	Sat Jul 27 10:02:35 2019 +0800
+++ b/src/hotspot/share/opto/superword.cpp	Mon Jul 29 13:57:54 2019 +0200
@@ -3296,7 +3296,14 @@
     Node* n = p->at(i);
     assert(n->is_Load(), "only meaningful for loads");
     if (!n->depends_only_on_test()) {
-      dep = LoadNode::Pinned;
+      if (n->as_Load()->has_unknown_control_dependency() &&
+          dep != LoadNode::Pinned) {
+        // Upgrade to unknown control...
+        dep = LoadNode::UnknownControl;
+      } else {
+        // Otherwise, we must pin it.
+        dep = LoadNode::Pinned;
+      }
     }
   }
   return dep;