changeset 47261:26c7b9602a1e mvt

8186716: [MVT] C2 crashes with "assert(!n->is_ValueType()) failed: value types should not be split through phis" Reviewed-by: roland
author thartmann
date Fri, 29 Sep 2017 15:12:26 +0200
parents b535d96f6fdb
children c5909c17df5c
files src/hotspot/share/opto/castnode.cpp src/hotspot/share/opto/compile.cpp src/hotspot/share/opto/compile.hpp src/hotspot/share/opto/loopopts.cpp src/hotspot/share/opto/macro.cpp src/hotspot/share/opto/node.cpp src/hotspot/share/opto/node.hpp src/hotspot/share/opto/phaseX.cpp src/hotspot/share/opto/split_if.cpp src/hotspot/share/opto/valuetypenode.cpp src/hotspot/share/opto/valuetypenode.hpp
diffstat 11 files changed, 108 insertions(+), 87 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/opto/castnode.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/castnode.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -411,7 +411,7 @@
   // ValueTypePtrNode from the call.
   if (can_reshape &&
       in(0) == NULL &&
-      phase->C->can_add_value_type_ptr() &&
+      phase->C->can_add_value_type() &&
       type()->isa_valuetypeptr() &&
       in(1) != NULL && in(1)->is_Proj() &&
       in(1)->in(0) != NULL && in(1)->in(0)->is_CallStaticJava() &&
--- a/src/hotspot/share/opto/compile.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/compile.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -405,11 +405,9 @@
       remove_expensive_node(n);
     }
   }
-  for (int i = value_type_ptr_count() - 1; i >= 0; i--) {
-    ValueTypePtrNode* vtptr = value_type_ptr(i);
-    if (!useful.member(vtptr)) {
-      remove_value_type_ptr(vtptr);
-    }
+  // Remove useless value type nodes
+  if (_value_type_nodes != NULL) {
+    _value_type_nodes->remove_useless_nodes(useful.member_set());
   }
   // clean up the late inline lists
   remove_useless_late_inlines(&_string_late_inlines, useful);
@@ -1185,7 +1183,7 @@
   _predicate_opaqs = new(comp_arena()) GrowableArray<Node*>(comp_arena(), 8,  0, NULL);
   _expensive_nodes = new(comp_arena()) GrowableArray<Node*>(comp_arena(), 8,  0, NULL);
   _range_check_casts = new(comp_arena()) GrowableArray<Node*>(comp_arena(), 8,  0, NULL);
-  _value_type_ptr_nodes = new(comp_arena()) GrowableArray<ValueTypePtrNode*>(comp_arena(), 8,  0, NULL);
+  _value_type_nodes = new (comp_arena()) Unique_Node_List(comp_arena());
   register_library_intrinsics();
 }
 
@@ -1971,22 +1969,30 @@
   assert(range_check_cast_count() == 0, "should be empty");
 }
 
-void Compile::add_value_type_ptr(ValueTypePtrNode* n) {
-  assert(can_add_value_type_ptr(), "too late");
-  assert(!_value_type_ptr_nodes->contains(n), "duplicate entry");
-  _value_type_ptr_nodes->append(n);
+void Compile::add_value_type(Node* n) {
+  assert(n->is_ValueTypeBase(), "unexpected node");
+  if (_value_type_nodes != NULL) {
+    _value_type_nodes->push(n);
+  }
 }
 
-void Compile::process_value_type_ptr_nodes(PhaseIterGVN &igvn) {
-  for (int i = value_type_ptr_count(); i > 0; i--) {
-    ValueTypePtrNode* vtptr = value_type_ptr(i-1);
-    // once all inlining is over otherwise debug info can get
-    // inconsistent
-    vtptr->make_scalar_in_safepoints(igvn.C->root(), &igvn);
-    igvn.replace_node(vtptr, vtptr->get_oop());
+void Compile::remove_value_type(Node* n) {
+  assert(n->is_ValueTypeBase(), "unexpected node");
+  if (_value_type_nodes != NULL) {
+    _value_type_nodes->remove(n);
   }
-  assert(value_type_ptr_count() == 0, "should be empty");
-  _value_type_ptr_nodes = NULL;
+}
+
+void Compile::process_value_types(PhaseIterGVN &igvn) {
+  // Make value types scalar in safepoints
+  while (_value_type_nodes->size() != 0) {
+    ValueTypeBaseNode* vt = _value_type_nodes->pop()->as_ValueTypeBase();
+    vt->make_scalar_in_safepoints(igvn.C->root(), &igvn);
+    if (vt->is_ValueTypePtr()) {
+      igvn.replace_node(vt, vt->get_oop());
+    }
+  }
+  _value_type_nodes = NULL;
   igvn.optimize();
 }
 
@@ -2237,8 +2243,9 @@
     igvn.optimize();
   }
 
-  if (value_type_ptr_count() > 0) {
-    process_value_type_ptr_nodes(igvn);
+  if (_value_type_nodes->size() > 0) {
+    // Do this once all inlining is over to avoid getting inconsistent debug info
+    process_value_types(igvn);
   }
 
   // Perform escape analysis
@@ -2385,25 +2392,9 @@
  print_method(PHASE_OPTIMIZE_FINISHED, 2);
 }
 
-// Fixme remove
-static void check_for_value_node(Node &n, void* C) {
-  if (n.is_ValueType()) {
-#ifdef ASSERT
-    ((Compile*)C)->method()->print_short_name();
-    tty->print_cr("");
-    n.dump(-1);
-    assert(false, "Unable to match ValueTypeNode");
-#endif
-    ((Compile*)C)->record_failure("Unable to match ValueTypeNode");
-  }
-}
-
 //------------------------------Code_Gen---------------------------------------
 // Given a graph, generate code for it
 void Compile::Code_Gen() {
-  // FIXME remove
-  root()->walk(Node::nop, check_for_value_node, this);
-
   if (failing()) {
     return;
   }
@@ -3382,18 +3373,14 @@
     }
     break;
   }
+#ifdef ASSERT
+  case Op_ValueTypePtr:
   case Op_ValueType: {
-    ValueTypeNode* vt = n->as_ValueType();
-    vt->make_scalar_in_safepoints(root(), NULL);
-    if (vt->outcnt() == 0) {
-      vt->disconnect_inputs(NULL, this);
-    }
+    n->dump(-1);
+    assert(false, "value type node was not removed");
     break;
   }
-  case Op_ValueTypePtr: {
-    ShouldNotReachHere();
-    break;
-  }
+#endif
   default:
     assert( !n->is_Call(), "" );
     assert( !n->is_Mem(), "" );
--- a/src/hotspot/share/opto/compile.hpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/compile.hpp	Fri Sep 29 15:12:26 2017 +0200
@@ -83,7 +83,7 @@
 class TypePtr;
 class TypeOopPtr;
 class TypeFunc;
-class ValueTypePtrNode;
+class ValueTypeBaseNode;
 class Unique_Node_List;
 class nmethod;
 class WarmCallInfo;
@@ -416,7 +416,7 @@
   GrowableArray<Node*>* _predicate_opaqs;       // List of Opaque1 nodes for the loop predicates.
   GrowableArray<Node*>* _expensive_nodes;       // List of nodes that are expensive to compute and that we'd better not let the GVN freely common
   GrowableArray<Node*>* _range_check_casts;     // List of CastII nodes with a range check dependency
-  GrowableArray<ValueTypePtrNode*>* _value_type_ptr_nodes; // List of ValueTypePtr nodes
+  Unique_Node_List*     _value_type_nodes;      // List of ValueType nodes
   ConnectionGraph*      _congraph;
 #ifndef PRODUCT
   IdealGraphPrinter*    _printer;
@@ -809,16 +809,11 @@
   // Remove all range check dependent CastIINodes.
   void  remove_range_check_casts(PhaseIterGVN &igvn);
 
-  void add_value_type_ptr(ValueTypePtrNode* n);
-  void remove_value_type_ptr(ValueTypePtrNode* n) {
-    if (_value_type_ptr_nodes->contains(n)) {
-      _value_type_ptr_nodes->remove(n);
-    }
-  }
-  ValueTypePtrNode* value_type_ptr(int idx) const { return _value_type_ptr_nodes->at(idx);  }
-  int   value_type_ptr_count()       const { return _value_type_ptr_nodes->length(); }
-  void  process_value_type_ptr_nodes(PhaseIterGVN &igvn);
-  bool can_add_value_type_ptr() const { return _value_type_ptr_nodes != NULL; }
+  // Keep track of value type nodes for later processing
+  void add_value_type(Node* n);
+  void remove_value_type(Node* n);
+  void process_value_types(PhaseIterGVN &igvn);
+  bool can_add_value_type() const { return _value_type_nodes != NULL; }
 
   // remove the opaque nodes that protect the predicates so that the unused checks and
   // uncommon traps will be eliminated from the graph.
--- a/src/hotspot/share/opto/loopopts.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/loopopts.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -1015,11 +1015,6 @@
         Node* m = n->fast_out(j);
         if (m->is_FastLock())
           return false;
-        if (m->is_ValueType()) {
-          // TODO this breaks optimizations!
-          // Value types should not be split through phis
-          //return false;
-        }
 #ifdef _LP64
         if (m->Opcode() == Op_ConvI2L)
           return false;
@@ -1372,6 +1367,7 @@
   // Remove multiple allocations of the same value type
   if (n->is_ValueType() && EliminateAllocations) {
     n->as_ValueType()->remove_redundant_allocations(&_igvn, this);
+    return; // n is now dead
   }
 
   // Check for Opaque2's who's loop has disappeared - who's input is in the
--- a/src/hotspot/share/opto/macro.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/macro.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -868,6 +868,7 @@
   //
   // Process the safepoint uses
   //
+  Unique_Node_List value_worklist;
   while (safepoints.length() > 0) {
     SafePointNode* sfpt = safepoints.pop();
     Node* mem = sfpt->memory();
@@ -996,6 +997,9 @@
         } else {
           field_val = transform_later(new DecodeNNode(field_val, field_val->get_ptr_type()));
         }
+      } else if (field_val->is_ValueType()) {
+        // Keep track of value types to scalarize them later
+        value_worklist.push(field_val);
       }
       sfpt->add_req(field_val);
     }
@@ -1009,6 +1013,11 @@
     _igvn._worklist.push(sfpt);
     safepoints_done.append_if_missing(sfpt); // keep it for rollback
   }
+  // Scalarize value types that were added to the safepoint
+  for (uint i = 0; i < value_worklist.size(); ++i) {
+    Node* vt = value_worklist.at(i);
+    vt->as_ValueType()->make_scalar_in_safepoints(C->root(), &_igvn);
+  }
   return true;
 }
 
--- a/src/hotspot/share/opto/node.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/node.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -504,9 +504,6 @@
   if (cast != NULL && cast->has_range_check()) {
     C->add_range_check_cast(cast);
   }
-  if (n->is_ValueTypePtr()) {
-    C->add_value_type_ptr(n->as_ValueTypePtr());
-  }
 
   n->set_idx(C->next_unique()); // Get new unique index as well
   debug_only( n->verify_construction() );
@@ -540,6 +537,9 @@
   if (n->is_SafePoint()) {
     n->as_SafePoint()->clone_replaced_nodes();
   }
+  if (n->is_ValueTypeBase()) {
+    C->add_value_type(n);
+  }
   return n;                     // Return the clone
 }
 
@@ -615,8 +615,8 @@
   if (cast != NULL && cast->has_range_check()) {
     compile->remove_range_check_cast(cast);
   }
-  if (is_ValueTypePtr()) {
-    compile->remove_value_type_ptr(as_ValueTypePtr());
+  if (is_ValueTypeBase()) {
+    compile->remove_value_type(this);
   }
 
   if (is_SafePoint()) {
@@ -1358,8 +1358,8 @@
       if (cast != NULL && cast->has_range_check()) {
         igvn->C->remove_range_check_cast(cast);
       }
-      if (dead->is_ValueTypePtr()) {
-        igvn->C->remove_value_type_ptr(dead->as_ValueTypePtr());
+      if (dead->is_ValueTypeBase()) {
+        igvn->C->remove_value_type(dead);
       }
       igvn->C->record_dead_node(dead->_idx);
       // Kill all inputs to the dead guy
--- a/src/hotspot/share/opto/node.hpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/node.hpp	Fri Sep 29 15:12:26 2017 +0200
@@ -141,8 +141,9 @@
 class Type;
 class TypeNode;
 class UnlockNode;
+class ValueTypeBaseNode;
 class ValueTypeNode;
-class ValueTypeBaseNode;
+class ValueTypePtrNode;
 class VectorNode;
 class LoadVectorNode;
 class StoreVectorNode;
--- a/src/hotspot/share/opto/phaseX.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/phaseX.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -1421,8 +1421,8 @@
       if (cast != NULL && cast->has_range_check()) {
         C->remove_range_check_cast(cast);
       }
-      if (dead->is_ValueTypePtr()) {
-        C->remove_value_type_ptr(dead->as_ValueTypePtr());
+      if (dead->is_ValueTypeBase()) {
+        C->remove_value_type(dead);
       }
     }
   } // while (_stack.is_nonempty())
--- a/src/hotspot/share/opto/split_if.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/split_if.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -27,6 +27,7 @@
 #include "opto/callnode.hpp"
 #include "opto/loopnode.hpp"
 #include "opto/movenode.hpp"
+#include "opto/valuetypenode.hpp"
 
 
 //------------------------------split_thru_region------------------------------
@@ -194,8 +195,16 @@
     rtype = TypeLong::INT;
   }
 
+  // Value types should not be split through Phis but each value input
+  // needs to be merged individually. At this point, value types should
+  // only be used by AllocateNodes. Try to remove redundant allocations
+  // and unlink the now dead value type node.
+  if (n->is_ValueType()) {
+    n->as_ValueType()->remove_redundant_allocations(&_igvn, this);
+    return true; // n is now dead
+  }
+
   // Now actually split-up this guy.  One copy per control path merging.
-  assert(!n->is_ValueType(), "value types should not be split through phis");
   Node *phi = PhiNode::make_blank(blk1, n);
   for( uint j = 1; j < blk1->req(); j++ ) {
     Node *x = n->clone();
--- a/src/hotspot/share/opto/valuetypenode.cpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/valuetypenode.cpp	Fri Sep 29 15:12:26 2017 +0200
@@ -201,6 +201,7 @@
 }
 
 void ValueTypeBaseNode::make_scalar_in_safepoints(Node* root, PhaseGVN* gvn) {
+  // Process all safepoint uses and scalarize value type
   Unique_Node_List worklist;
   for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
     Node* u = fast_out(i);
@@ -213,9 +214,9 @@
       --i; imax -= nb;
     }
   }
-
-  for (uint next = 0; next < worklist.size(); ++next) {
-    Node* vt = worklist.at(next);
+  // Now scalarize non-flattened fields
+  for (uint i = 0; i < worklist.size(); ++i) {
+    Node* vt = worklist.at(i);
     vt->as_ValueType()->make_scalar_in_safepoints(root, gvn);
   }
 }
@@ -469,7 +470,7 @@
 ValueTypeNode* ValueTypeNode::make(PhaseGVN& gvn, ciValueKlass* klass) {
   // Create a new ValueTypeNode with uninitialized values and NULL oop
   const TypeValueType* type = TypeValueType::make(klass);
-  return new ValueTypeNode(type, gvn.zerocon(T_VALUETYPE));
+  return new ValueTypeNode(type, gvn.zerocon(T_VALUETYPE), gvn.C);
 }
 
 Node* ValueTypeNode::make_default(PhaseGVN& gvn, ciValueKlass* vk) {
@@ -493,7 +494,7 @@
   // Create and initialize a ValueTypeNode by loading all field
   // values from a heap-allocated version and also save the oop.
   const TypeValueType* type = gvn.type(oop)->is_valuetypeptr()->value_type();
-  ValueTypeNode* vt = new ValueTypeNode(type, oop);
+  ValueTypeNode* vt = new ValueTypeNode(type, oop, gvn.C);
 
   if (null_check && !vt->is_allocated(&gvn)) {
     // Add oop null check
@@ -769,6 +770,30 @@
     phase->lazy_replace(projs.catchall_catchproj, phase->C->top());
     phase->lazy_replace(projs.resproj, phase->C->top());
   }
+
+  // Process users
+  for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
+    Node* out = fast_out(i);
+    if (out->isa_ValueType() != NULL) {
+      // Recursively process value type users
+      out->as_ValueType()->remove_redundant_allocations(igvn, phase);
+      --i; --imax;
+    } else if (out->isa_Allocate() != NULL) {
+      // Unlink AllocateNode
+      assert(out->in(AllocateNode::ValueNode) == this, "should be linked");
+      igvn->replace_input_of(out, AllocateNode::ValueNode, NULL);
+      --i; --imax;
+    } else {
+#ifdef ASSERT
+      // The value type should not have any other users at this time
+      out->dump();
+      assert(false, "unexpected user of value type");
+#endif
+    }
+  }
+
+  // Should be dead now
+  igvn->remove_dead_node(this);
 }
 
 #ifndef PRODUCT
--- a/src/hotspot/share/opto/valuetypenode.hpp	Fri Sep 29 09:09:14 2017 +0200
+++ b/src/hotspot/share/opto/valuetypenode.hpp	Fri Sep 29 15:12:26 2017 +0200
@@ -32,9 +32,10 @@
 
 class ValueTypeBaseNode : public TypeNode {
 protected:
-  ValueTypeBaseNode(const Type* t, int nb_fields)
+  ValueTypeBaseNode(const Type* t, int nb_fields, Compile* C)
     : TypeNode(t, nb_fields) {
     init_class_id(Class_ValueTypeBase);
+    C->add_value_type(this);
   }
 
   enum { Control,   // Control input
@@ -93,8 +94,8 @@
 class ValueTypeNode : public ValueTypeBaseNode {
   friend class ValueTypeBaseNode;
 private:
-  ValueTypeNode(const TypeValueType* t, Node* oop)
-    : ValueTypeBaseNode(t, Values + t->value_klass()->nof_declared_nonstatic_fields()) {
+  ValueTypeNode(const TypeValueType* t, Node* oop, Compile* C)
+    : ValueTypeBaseNode(t, Values + t->value_klass()->nof_declared_nonstatic_fields(), C) {
     init_class_id(Class_ValueType);
     init_req(Oop, oop);
   }
@@ -146,21 +147,19 @@
   const TypeValueTypePtr* value_type_ptr() const { return bottom_type()->isa_valuetypeptr(); }
 
   ValueTypePtrNode(ciValueKlass* vk, Node* oop, Compile* C)
-    : ValueTypeBaseNode(TypeValueTypePtr::make(TypePtr::NotNull, vk), Values + vk->nof_declared_nonstatic_fields()) {
+    : ValueTypeBaseNode(TypeValueTypePtr::make(TypePtr::NotNull, vk), Values + vk->nof_declared_nonstatic_fields(), C) {
     init_class_id(Class_ValueTypePtr);
     init_req(Oop, oop);
-    C->add_value_type_ptr(this);
   }
 public:
 
   ValueTypePtrNode(ValueTypeNode* vt, Node* oop, Compile* C)
-    : ValueTypeBaseNode(TypeValueTypePtr::make(vt->type()->is_valuetype())->cast_to_ptr_type(TypePtr::NotNull), vt->req()) {
+    : ValueTypeBaseNode(TypeValueTypePtr::make(vt->type()->is_valuetype())->cast_to_ptr_type(TypePtr::NotNull), vt->req(), C) {
     init_class_id(Class_ValueTypePtr);
     for (uint i = Oop+1; i < vt->req(); i++) {
       init_req(i, vt->in(i));
     }
     init_req(Oop, oop);
-    C->add_value_type_ptr(this);
   }
 
   static ValueTypePtrNode* make(GraphKit* kit, ciValueKlass* vk, CallNode* call);