8356599: [lworld] C2 incorrectly folds array object klass load for atomic arrays

8355941: [lworld] TestArrayAccessDeopt.java fails due to unexpected deoptimization

Author: TobiHartmann

src/hotspot/share/ci/ciArrayKlass.cpp

@@ -117,9 +117,6 @@ ciArrayKlass* ciArrayKlass::make(ciType* element_type, bool flat, bool null_free
       if (flat && vk->maybe_flat_in_array()) {
         LayoutKind lk;
         if (null_free) {
-          if (vk->is_naturally_atomic()) {
-            atomic = vk->has_atomic_layout();
-          }
           if (!atomic && !vk->has_non_atomic_layout()) {
             // TODO 8350865 Impossible type
             lk = vk->has_atomic_layout() ? LayoutKind::ATOMIC_FLAT : LayoutKind::NULLABLE_ATOMIC_FLAT;

src/hotspot/share/ci/ciKlass.cpp

@@ -251,6 +251,11 @@ void ciKlass::print_impl(outputStream* st) {
   st->print(" name=");
   print_name_on(st);
   st->print(" loaded=%s", (is_loaded() ? "true" : "false"));
+  GUARDED_VM_ENTRY(
+    if (is_flat_array_klass()) {
+      st->print(" layout_kind=%d", (int)((FlatArrayKlass*)get_Klass())->layout_kind());
+    }
+  )
 }
 
 // ------------------------------------------------------------------

src/hotspot/share/opto/graphKit.cpp

@@ -1227,12 +1227,12 @@ Node* GraphKit::ConvL2I(Node* offset) {
 }
 
 //-------------------------load_object_klass-----------------------------------
-Node* GraphKit::load_object_klass(Node* obj) {
+Node* GraphKit::load_object_klass(Node* obj, bool fold_for_arrays) {
   // Special-case a fresh allocation to avoid building nodes:
   Node* akls = AllocateNode::Ideal_klass(obj, &_gvn);
   if (akls != nullptr)  return akls;
   Node* k_adr = basic_plus_adr(obj, oopDesc::klass_offset_in_bytes());
-  return _gvn.transform(LoadKlassNode::make(_gvn, immutable_memory(), k_adr, TypeInstPtr::KLASS, TypeInstKlassPtr::OBJECT));
+  return _gvn.transform(LoadKlassNode::make(_gvn, immutable_memory(), k_adr, TypeInstPtr::KLASS, TypeInstKlassPtr::OBJECT, fold_for_arrays));
 }
 
 //-------------------------load_array_length-----------------------------------
@@ -3787,7 +3787,8 @@ Node* GraphKit::null_free_atomic_array_test(Node* array, ciInlineKlass* vk) {
     return intcon(0); // Never atomic
   }
 
-  Node* array_klass = load_object_klass(array);
+  // TODO 8350865 Don't fold this klass load because atomicity is currently not included in the typesystem
+  Node* array_klass = load_object_klass(array, /* fold_for_arrays = */ false);
   int layout_kind_offset = in_bytes(FlatArrayKlass::layout_kind_offset());
   Node* layout_kind_addr = basic_plus_adr(array_klass, array_klass, layout_kind_offset);
   Node* layout_kind = make_load(nullptr, layout_kind_addr, TypeInt::INT, T_INT, MemNode::unordered);

src/hotspot/share/opto/graphKit.hpp

@@ -355,7 +355,7 @@ class GraphKit : public Phase {
   Node* ConvI2UL(Node* offset);
   Node* ConvL2I(Node* offset);
   // Find out the klass of an object.
-  Node* load_object_klass(Node* object);
+  Node* load_object_klass(Node* object, bool fold_for_arrays = true);
   // Find out the length of an array.
   Node* load_array_length(Node* array);
   // Cast array allocation's length as narrow as possible.

src/hotspot/share/opto/inlinetypenode.cpp

@@ -1302,9 +1302,9 @@ InlineTypeNode* InlineTypeNode::make_from_flat_impl(GraphKit* kit, ciInlineKlass
   bool null_free = (null_marker_offset == -1);
   InlineTypeNode* vt = make_uninitialized(kit->gvn(), vk, null_free);
 
+  bool is_array = (kit->gvn().type(obj)->isa_aryptr() != nullptr);
   if (atomic) {
     // Read atomically and convert from payload
-    bool is_array = (kit->gvn().type(obj)->isa_aryptr() != nullptr);
 #ifdef ASSERT
     bool is_naturally_atomic = (!is_array && vk->is_empty()) || (null_free && vk->nof_declared_nonstatic_fields() == 1);
     assert(!is_naturally_atomic, "No atomic access required");
@@ -1342,21 +1342,22 @@ InlineTypeNode* InlineTypeNode::make_from_flat_impl(GraphKit* kit, ciInlineKlass
       Node* bol = kit->null_free_array_test(obj); // Argument evaluation order is undefined in C++ and since this sets control, it needs to come first
       IfNode* iff = kit->create_and_map_if(kit->control(), bol, PROB_FAIR, COUNT_UNKNOWN);
 
-      kit->set_control(kit->IfFalse(iff));
-      region->init_req(1, kit->control());
-
       // Nullable
+      kit->set_control(kit->IfFalse(iff));
       if (!kit->stopped()) {
         assert(!null_free && vk->has_nullable_atomic_layout(), "Flat array can't be nullable");
-        Node* load = kit->access_load_at(obj, ptr, TypeRawPtr::BOTTOM, val_type, bt, is_array ? (decorators | IS_ARRAY) : decorators, kit->control());
+
+        Node* cast = obj;
+        Node* adr = kit->flat_array_element_address(cast, idx, vk, /* null_free */ false, /* not_null_free */ true, /* atomic */ true);
+        Node* load = kit->access_load_at(cast, adr, TypeRawPtr::BOTTOM, val_type, bt, is_array ? (decorators | IS_ARRAY) : decorators, kit->control());
         payload->init_req(1, load);
         mem->init_req(1, kit->reset_memory());
         io->init_req(1, kit->i_o());
       }
-
-      kit->set_control(kit->IfTrue(iff));
+      region->init_req(1, kit->control());
 
       // Null-free
+      kit->set_control(kit->IfTrue(iff));
       if (!kit->stopped()) {
         kit->set_all_memory(input_memory_state);
 
@@ -1446,7 +1447,6 @@ InlineTypeNode* InlineTypeNode::make_from_flat_impl(GraphKit* kit, ciInlineKlass
   holder_offset -= vk->payload_offset();
 
   if (!null_free) {
-    bool is_array = (kit->gvn().type(obj)->isa_aryptr() != nullptr);
     Node* adr = kit->basic_plus_adr(obj, ptr, null_marker_offset);
     Node* nm_value = kit->access_load_at(obj, adr, TypeRawPtr::BOTTOM, TypeInt::BOOL, T_BOOLEAN, is_array ? (decorators | IS_ARRAY) : decorators);
     vt->set_req(IsInit, nm_value);

src/hotspot/share/opto/library_call.cpp

@@ -4599,7 +4599,7 @@ bool LibraryCallKit::inline_newArray(bool null_free, bool atomic) {
 
         ciArrayKlass* array_klass = ciArrayKlass::make(t, flat, null_free, atomic);
         if (array_klass->is_loaded() && array_klass->element_klass()->as_inline_klass()->is_initialized()) {
-          const TypeAryKlassPtr* array_klass_type = TypeKlassPtr::make(array_klass, Type::trust_interfaces)->is_aryklassptr();
+          const TypeAryKlassPtr* array_klass_type = TypeAryKlassPtr::make(array_klass, Type::trust_interfaces);
           if (null_free) {
             if (init_val->is_InlineType()) {
               if (array_klass_type->is_flat() && init_val->as_InlineType()->is_all_zero(&gvn(), /* flat */ true)) {
@@ -5665,6 +5665,7 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
     PhiNode*    result_mem = new PhiNode(result_reg, Type::MEMORY, TypePtr::BOTTOM);
     record_for_igvn(result_reg);
 
+    // TODO 8350865 For arrays, this might be folded and then not account for atomic arrays
     Node* obj_klass = load_object_klass(obj);
     // We only go to the fast case code if we pass a number of guards.
     // The paths which do not pass are accumulated in the slow_region.

src/hotspot/share/opto/memnode.cpp

@@ -2619,27 +2619,27 @@ Node* LoadNNode::Ideal(PhaseGVN* phase, bool can_reshape) {
 //=============================================================================
 //----------------------------LoadKlassNode::make------------------------------
 // Polymorphic factory method:
-Node* LoadKlassNode::make(PhaseGVN& gvn, Node* mem, Node* adr, const TypePtr* at, const TypeKlassPtr* tk) {
+Node* LoadKlassNode::make(PhaseGVN& gvn, Node* mem, Node* adr, const TypePtr* at, const TypeKlassPtr* tk, bool fold_for_arrays) {
   // sanity check the alias category against the created node type
   const TypePtr* adr_type = adr->bottom_type()->isa_ptr();
   assert(adr_type != nullptr, "expecting TypeKlassPtr");
 #ifdef _LP64
   if (adr_type->is_ptr_to_narrowklass()) {
     assert(UseCompressedClassPointers, "no compressed klasses");
-    Node* load_klass = gvn.transform(new LoadNKlassNode(mem, adr, at, tk->make_narrowklass(), MemNode::unordered));
+    Node* load_klass = gvn.transform(new LoadNKlassNode(mem, adr, at, tk->make_narrowklass(), MemNode::unordered, fold_for_arrays));
     return new DecodeNKlassNode(load_klass, load_klass->bottom_type()->make_ptr());
   }
 #endif
   assert(!adr_type->is_ptr_to_narrowklass() && !adr_type->is_ptr_to_narrowoop(), "should have got back a narrow oop");
-  return new LoadKlassNode(mem, adr, at, tk, MemNode::unordered);
+  return new LoadKlassNode(mem, adr, at, tk, MemNode::unordered, fold_for_arrays);
 }
 
 //------------------------------Value------------------------------------------
 const Type* LoadKlassNode::Value(PhaseGVN* phase) const {
-  return klass_value_common(phase);
+  return klass_value_common(phase, _fold_for_arrays);
 }
 
-const Type* LoadNode::klass_value_common(PhaseGVN* phase) const {
+const Type* LoadNode::klass_value_common(PhaseGVN* phase, bool fold_for_arrays) const {
   // Either input is TOP ==> the result is TOP
   const Type *t1 = phase->type( in(MemNode::Memory) );
   if (t1 == Type::TOP)  return Type::TOP;
@@ -2699,7 +2699,7 @@ const Type* LoadNode::klass_value_common(PhaseGVN* phase) const {
 
   // Check for loading klass from an array
   const TypeAryPtr* tary = tp->isa_aryptr();
-  if (tary != nullptr &&
+  if (tary != nullptr && fold_for_arrays &&
       tary->offset() == oopDesc::klass_offset_in_bytes()) {
     return tary->as_klass_type(true);
   }
@@ -2814,7 +2814,7 @@ LoadNode* LoadNode::clone_pinned() const {
 
 //------------------------------Value------------------------------------------
 const Type* LoadNKlassNode::Value(PhaseGVN* phase) const {
-  const Type *t = klass_value_common(phase);
+  const Type *t = klass_value_common(phase, _fold_for_arrays);
   if (t == Type::TOP)
     return t;
 

src/hotspot/share/opto/memnode.hpp

@@ -265,7 +265,7 @@ class LoadNode : public MemNode {
   virtual const Type* Value(PhaseGVN* phase) const;
 
   // Common methods for LoadKlass and LoadNKlass nodes.
-  const Type* klass_value_common(PhaseGVN* phase) const;
+  const Type* klass_value_common(PhaseGVN* phase, bool fold_for_arrays) const;
   Node* klass_identity_common(PhaseGVN* phase);
 
   virtual uint ideal_reg() const;
@@ -527,9 +527,17 @@ class LoadNNode : public LoadNode {
 //------------------------------LoadKlassNode----------------------------------
 // Load a Klass from an object
 class LoadKlassNode : public LoadPNode {
+  bool _fold_for_arrays;
+
+  virtual uint size_of() const { return sizeof(*this); }
+  virtual uint hash() const { return LoadNode::hash() + _fold_for_arrays; }
+  virtual bool cmp( const Node &n ) const {
+    return _fold_for_arrays == ((LoadKlassNode&)n)._fold_for_arrays && LoadNode::cmp(n);
+  }
+
 private:
-  LoadKlassNode(Node* mem, Node* adr, const TypePtr* at, const TypeKlassPtr* tk, MemOrd mo)
-    : LoadPNode(nullptr, mem, adr, at, tk, mo) {}
+  LoadKlassNode(Node* mem, Node* adr, const TypePtr* at, const TypeKlassPtr* tk, MemOrd mo, bool fold_for_arrays)
+    : LoadPNode(nullptr, mem, adr, at, tk, mo), _fold_for_arrays(fold_for_arrays) {}
 
 public:
   virtual int Opcode() const;
@@ -539,7 +547,7 @@ class LoadKlassNode : public LoadPNode {
 
   // Polymorphic factory method:
   static Node* make(PhaseGVN& gvn, Node* mem, Node* adr, const TypePtr* at,
-                    const TypeKlassPtr* tk = TypeInstKlassPtr::OBJECT);
+                    const TypeKlassPtr* tk = TypeInstKlassPtr::OBJECT, bool fold_for_arrays = true);
 };
 
 //------------------------------LoadNKlassNode---------------------------------
@@ -551,10 +559,18 @@ class LoadKlassNode : public LoadPNode {
 // extract the actual class pointer. C2's type system is agnostic on whether the
 // input address directly points into the class pointer.
 class LoadNKlassNode : public LoadNNode {
+  bool _fold_for_arrays;
+
+  virtual uint size_of() const { return sizeof(*this); }
+  virtual uint hash() const { return LoadNode::hash() + _fold_for_arrays; }
+  virtual bool cmp( const Node &n ) const {
+    return _fold_for_arrays == ((LoadNKlassNode&)n)._fold_for_arrays && LoadNode::cmp(n);
+  }
+
 private:
-  friend Node* LoadKlassNode::make(PhaseGVN&, Node*, Node*, const TypePtr*, const TypeKlassPtr*);
-  LoadNKlassNode(Node* mem, Node* adr, const TypePtr* at, const TypeNarrowKlass* tk, MemOrd mo)
-    : LoadNNode(nullptr, mem, adr, at, tk, mo) {}
+  friend Node* LoadKlassNode::make(PhaseGVN&, Node*, Node*, const TypePtr*, const TypeKlassPtr*, bool fold_for_arrays);
+  LoadNKlassNode(Node* mem, Node* adr, const TypePtr* at, const TypeNarrowKlass* tk, MemOrd mo, bool fold_for_arrays)
+    : LoadNNode(nullptr, mem, adr, at, tk, mo), _fold_for_arrays(fold_for_arrays) {}
 
 public:
   virtual int Opcode() const;

src/hotspot/share/opto/type.cpp

@@ -5191,9 +5191,6 @@ const TypeAryPtr* TypeAryPtr::make(PTR ptr, const TypeAry *ary, ciKlass* k, bool
       k->as_obj_array_klass()->base_element_klass()->is_interface()) {
     k = nullptr;
   }
-  if (k != nullptr && k->is_flat_array_klass() && !ary->_flat) {
-    k = nullptr;
-  }
   return (TypeAryPtr*)(new TypeAryPtr(ptr, nullptr, ary, k, xk, offset, field_offset, instance_id, false, speculative, inline_depth))->hashcons();
 }
 
@@ -5210,9 +5207,6 @@ const TypeAryPtr* TypeAryPtr::make(PTR ptr, ciObject* o, const TypeAry *ary, ciK
       k->as_obj_array_klass()->base_element_klass()->is_interface()) {
     k = nullptr;
   }
-  if (k != nullptr && k->is_flat_array_klass() && !ary->_flat) {
-    k = nullptr;
-  }
   return (TypeAryPtr*)(new TypeAryPtr(ptr, o, ary, k, xk, offset, field_offset, instance_id, is_autobox_cache, speculative, inline_depth))->hashcons();
 }
 
@@ -6804,7 +6798,7 @@ const TypeAryKlassPtr* TypeAryKlassPtr::make(PTR ptr, ciKlass* k, Offset offset,
   } else if (k->is_flat_array_klass()) {
     ciKlass* eklass = k->as_flat_array_klass()->element_klass();
     const TypeKlassPtr* etype = TypeKlassPtr::make(eklass, interface_handling)->cast_to_exactness(false);
-    return TypeAryKlassPtr::make(ptr, etype, nullptr, offset, not_flat, not_null_free, flat, null_free);
+    return TypeAryKlassPtr::make(ptr, etype, k, offset, not_flat, not_null_free, flat, null_free);
   } else {
     ShouldNotReachHere();
     return nullptr;
@@ -6920,9 +6914,7 @@ ciKlass* TypeAryPtr::exact_klass_helper() const {
     if (k == nullptr) {
       return nullptr;
     }
-    // TODO 8350865 We assume atomic if the atomic layout is available
-    bool atomic = k->is_inlinetype() && (is_null_free() ? k->as_inline_klass()->has_atomic_layout() : k->as_inline_klass()->has_nullable_atomic_layout());
-    k = ciArrayKlass::make(k, is_flat(), is_null_free(), atomic);
+    k = ciArrayKlass::make(k, is_flat(), is_null_free());
     return k;
   }
 
@@ -7320,9 +7312,12 @@ ciKlass* TypeAryKlassPtr::exact_klass_helper() const {
     if (k == nullptr) {
       return nullptr;
     }
-    // TODO 8350865 We assume atomic if the atomic layout is available
-    bool atomic = k->is_inlinetype() && (is_null_free() ? k->as_inline_klass()->has_atomic_layout() : k->as_inline_klass()->has_nullable_atomic_layout());
-    k = ciArrayKlass::make(k, is_flat(), is_null_free(), atomic);
+    // TODO 8350865 LibraryCallKit::inline_newArray passes a constant TypeAryKlassPtr to GraphKit::new_array
+    // As long as atomicity is not tracked by TypeAryKlassPtr, don't re-compute it here to avoid loosing atomicity information
+    if (k->is_inlinetype() && _klass != nullptr) {
+      return _klass;
+    }
+    k = ciArrayKlass::make(k, is_flat(), is_null_free());
     return k;
   }
 

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestArrayAccessDeopt.java

@@ -24,6 +24,7 @@
 /**
  * @test
  * @summary Verify that certain array accesses do not trigger deoptimization.
+ * @requires vm.debug == true
  * @library /test/lib
  * @enablePreview
  * @modules java.base/jdk.internal.value
@@ -100,6 +101,7 @@ static public void main(String[] args) throws Exception {
                             "-XX:CompileCommand=quiet", "-XX:CompileCommand=compileonly,TestArrayAccessDeopt::test*", "-XX:-UseArrayLoadStoreProfile",
                             "-XX:+TraceDeoptimization", "-Xbatch", "-XX:-MonomorphicArrayCheck", "-Xmixed", "-XX:+ProfileInterpreter", "TestArrayAccessDeopt", "run"};
             OutputAnalyzer oa = ProcessTools.executeTestJava(arg);
+            oa.shouldHaveExitValue(0);
             String output = oa.getOutput();
             oa.shouldNotContain("UNCOMMON TRAP");
         } else {