Refactoring and a fix

TobiHartmann · Dec 12, 2024 · 496f2b8 · 496f2b8
1 parent 9110592
commit 496f2b8
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 43 deletions.
diff --git a/src/hotspot/share/opto/inlinetypenode.cpp b/src/hotspot/share/opto/inlinetypenode.cpp
@@ -561,18 +561,18 @@ void InlineTypeNode::store_flat(GraphKit* kit, Node* base, Node* ptr, ciInstance
   if (kit->gvn().type(base)->isa_aryptr()) {
     kit->C->set_flat_accesses();
   }
-  // The inline type is embedded into the object without an oop header. Subtract the
-  // offset of the first field to account for the missing header when storing the values.
-  if (holder == nullptr) {
-    holder = inline_klass();
-  }
   if (null_marker_offset != -1) {
     // Nullable flat field, store the null marker
-    Node* adr = kit->basic_plus_adr(base, null_marker_offset);
+    Node* adr = kit->basic_plus_adr(base, ptr, null_marker_offset);
     const TypePtr* adr_type = kit->gvn().type(adr)->isa_ptr();
     int alias_idx = kit->C->get_alias_index(adr_type);
     kit->store_to_memory(kit->control(), adr, get_is_init(), T_BOOLEAN, alias_idx, MemNode::unordered);
   }
+  // The inline type is embedded into the object without an oop header. Subtract the
+  // offset of the first field to account for the missing header when storing the values.
+  if (holder == nullptr) {
+    holder = inline_klass();
+  }
   holder_offset -= inline_klass()->first_field_offset();
   store(kit, base, ptr, holder, holder_offset, -1, decorators);
 }
@@ -1030,23 +1030,20 @@ InlineTypeNode* InlineTypeNode::make_from_flat_impl(GraphKit* kit, ciInlineKlass
   if (kit->gvn().type(obj)->isa_aryptr()) {
     kit->C->set_flat_accesses();
   }
-
-  bool null_free = (null_marker_offset == -1);
-
   // Create and initialize an InlineTypeNode by loading all field values from
   // a flat inline type field at 'holder_offset' or from an inline type array.
+  bool null_free = (null_marker_offset == -1);
   InlineTypeNode* vt = make_uninitialized(kit->gvn(), vk, null_free);
-  // The inline type is flattened into the object without an oop header. Subtract the
-  // offset of the first field to account for the missing header when loading the values.
-  holder_offset -= vk->first_field_offset();
-
-  if (null_marker_offset != -1) {
+  if (!null_free) {
     // Nullable flat field, read the null marker
-    Node* adr = kit->basic_plus_adr(obj, null_marker_offset);
+    Node* adr = kit->basic_plus_adr(obj, ptr, null_marker_offset);
     Node* is_init = kit->make_load(nullptr, adr, TypeInt::BOOL, T_BOOLEAN, MemNode::unordered);
     vt->set_req(IsInit, is_init);
   }
 
+  // The inline type is flattened into the object without an oop header. Subtract the
+  // offset of the first field to account for the missing header when loading the values.
+  holder_offset -= vk->first_field_offset();
   vt->load(kit, obj, ptr, holder, visited, holder_offset, decorators);
   assert(vt->is_loaded(&kit->gvn()) != obj, "holder oop should not be used as flattened inline type oop");
   return kit->gvn().transform(vt)->as_InlineType();

diff --git a/src/hotspot/share/opto/macro.cpp b/src/hotspot/share/opto/macro.cpp
@@ -567,7 +567,7 @@ Node *PhaseMacroExpand::value_from_mem(Node *sfpt_mem, Node *sfpt_ctl, BasicType
   return nullptr;
 }
 
-// Search the last value stored into the inline type's fields.
+// Search the last value stored into the inline type's fields (for flat arrays).
 Node* PhaseMacroExpand::inline_type_from_mem(Node* mem, Node* ctl, ciInlineKlass* vk, const TypeAryPtr* adr_type, int offset, AllocateNode* alloc) {
   // Subtract the offset of the first field to account for the missing oop header
   offset -= vk->first_field_offset();
@@ -579,8 +579,10 @@ Node* PhaseMacroExpand::inline_type_from_mem(Node* mem, Node* ctl, ciInlineKlass
     int field_offset = offset + vt->field_offset(i);
     Node* value = nullptr;
     if (vt->field_is_flat(i)) {
-      // TODO could be null
-      assert(vt->field_is_null_free(i), "FIXME");
+      if (!vt->field_is_null_free(i)) {
+        // TODO with atomic accesses we should not hit this anymore
+        return nullptr;
+      }
       value = inline_type_from_mem(mem, ctl, field_type->as_inline_klass(), adr_type, field_offset, alloc);
     } else {
       const Type* ft = Type::get_const_type(field_type);

diff --git a/src/hotspot/share/opto/type.cpp b/src/hotspot/share/opto/type.cpp
@@ -2199,7 +2199,6 @@ static void collect_inline_fields(ciInlineKlass* vk, const Type** field_array, u
     if (field->is_flat()) {
       collect_inline_fields(field->type()->as_inline_klass(), field_array, pos);
       if (!field->is_null_free()) {
-        // TODO add is_init field at the end
         field_array[pos++] = Type::get_const_basic_type(T_BOOLEAN);
       }
     } else {
@@ -2238,7 +2237,6 @@ const TypeTuple *TypeTuple::make_range(ciSignature* sig, InterfaceHandling inter
       uint pos = TypeFunc::Parms;
       field_array[pos++] = get_const_type(return_type); // Oop might be null when returning as fields
       collect_inline_fields(return_type->as_inline_klass(), field_array, pos);
-      // TODO move this in
       // InlineTypeNode::IsInit field used for null checking
       field_array[pos++] = get_const_basic_type(T_BOOLEAN);
       assert(pos == (TypeFunc::Parms + arg_cnt), "out of bounds");

diff --git a/src/hotspot/share/runtime/deoptimization.cpp b/src/hotspot/share/runtime/deoptimization.cpp
@@ -1512,16 +1512,9 @@ static int compare(ReassignedField* left, ReassignedField* right) {
 
 // Restore fields of an eliminated instance object using the same field order
 // returned by HotSpotResolvedObjectTypeImpl.getInstanceFields(true)
-static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap* reg_map, ObjectValue* sv, int svIndex, oop obj, bool skip_internal, int base_offset, GrowableArray<ReassignedField>* null_markers, TRAPS) {
+static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap* reg_map, ObjectValue* sv, int svIndex, oop obj, bool skip_internal, int base_offset, GrowableArray<int>* null_marker_offsets, TRAPS) {
   GrowableArray<ReassignedField>* fields = new GrowableArray<ReassignedField>();
-  bool doIt = false;
-  if (null_markers == nullptr) {
-    doIt = true;
-    // Null markers are no real fields..
-    null_markers = new GrowableArray<ReassignedField>();
-  }
   InstanceKlass* ik = klass;
-  // TODO can we use a hierachical field stream here?
   while (ik != nullptr) {
     for (AllFieldStream fs(ik); !fs.done(); fs.next()) {
       if (!fs.access_flags().is_static() && (!skip_internal || !fs.field_flags().is_injected())) {
@@ -1533,18 +1526,19 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap
           field._is_null_free = fs.is_null_free_inline_type();
           // Resolve klass of flat inline type field
           field._klass = InlineKlass::cast(klass->get_inline_type_field_klass(fs.index()));
-        } else {
-          // TODO?
-          if (fs.is_null_free_inline_type()) {
-            field._type = T_OBJECT;  // Can be removed once Q-descriptors have been removed.
-          }
         }
         fields->append(field);
       }
     }
     ik = ik->superklass();
   }
   fields->sort(compare);
+  // Keep track of null marker offset for flat fields
+  bool set_null_markers = false;
+  if (null_marker_offsets == nullptr) {
+    set_null_markers = true;
+    null_marker_offsets = new GrowableArray<int>();
+  }
   for (int i = 0; i < fields->length(); i++) {
     BasicType type = fields->at(i)._type;
     int offset = base_offset + fields->at(i)._offset;
@@ -1554,15 +1548,11 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap
       InstanceKlass* vk = fields->at(i)._klass;
       assert(vk != nullptr, "must be resolved");
       offset -= InlineKlass::cast(vk)->first_field_offset(); // Adjust offset to omit oop header
-      svIndex = reassign_fields_by_klass(vk, fr, reg_map, sv, svIndex, obj, skip_internal, offset, null_markers, CHECK_0);
-
+      svIndex = reassign_fields_by_klass(vk, fr, reg_map, sv, svIndex, obj, skip_internal, offset, null_marker_offsets, CHECK_0);
       if (!fields->at(i)._is_null_free) {
         int nm_offset = offset + InlineKlass::cast(vk)->null_marker_offset();
-        ReassignedField field;
-        field._offset = nm_offset;
-        null_markers->append(field);
+        null_marker_offsets->append(nm_offset);
       }
-
       continue; // Continue because we don't need to increment svIndex
     }
     ScopeValue* scope_field = sv->field_at(svIndex);
@@ -1640,9 +1630,10 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap
     }
     svIndex++;
   }
-  if (doIt) {
-    for (int i = 0; i < null_markers->length(); ++i) {
-      int offset = null_markers->at(i)._offset;
+  if (set_null_markers) {
+    // The null marker values come after all the field values in the debug info
+    for (int i = 0; i < null_marker_offsets->length(); ++i) {
+      int offset = null_marker_offsets->at(i);
       jbyte is_init = (jbyte)StackValue::create_stack_value(fr, reg_map, sv->field_at(svIndex++))->get_jint();
       obj->byte_field_put(offset, is_init);
     }

diff --git a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFieldNullMarkers.java b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFieldNullMarkers.java
@@ -23,6 +23,7 @@
 
 package compiler.valhalla.inlinetypes;
 
+import jdk.internal.value.ValueClass;
 import jdk.internal.vm.annotation.ImplicitlyConstructible;
 import jdk.internal.vm.annotation.LooselyConsistentValue;
 import jdk.internal.vm.annotation.NullRestricted;
@@ -36,7 +37,8 @@
  * @library /test/lib /
  * @requires (os.simpleArch == "x64" | os.simpleArch == "aarch64")
  * @enablePreview
- * @modules java.base/jdk.internal.vm.annotation
+ * @modules java.base/jdk.internal.value
+ *          java.base/jdk.internal.vm.annotation
  * @run main/othervm -Xbatch -XX:+NullableFieldFlattening
  *                   compiler.valhalla.inlinetypes.TestFieldNullMarkers
  * @run main/othervm -Xbatch -XX:+NullableFieldFlattening
@@ -473,6 +475,28 @@ public void testDeopt5(MyValue7 val5, MyValue7 val6, MyValue7 val7, MyValue7 val
         }
     }
 
+    @ImplicitlyConstructible
+    @LooselyConsistentValue
+    static value class MyHolderClass8 {
+        MyValue8 val8;
+
+        public MyHolderClass8(MyValue8 val8) {
+            this.val8 = val8;
+        }
+    }
+
+    // Test support for null markers in scalar replaced flat (null-free) array
+    public static void testFlatArray1(boolean trap) {
+        MyHolderClass8[] array = (MyHolderClass8[])ValueClass.newNullRestrictedArray(MyHolderClass8.class, 2);
+        MyValue8 val8 = new MyValue8((byte)42);
+        array[0] = new MyHolderClass8(val8);
+        array[1] = new MyHolderClass8(null);
+        if (trap) {
+            Asserts.assertEQ(array[0].val8, val8);
+            Asserts.assertEQ(array[1].val8, null);
+        }
+    }
+
     public static void main(String[] args) {
         TestFieldNullMarkers t = new TestFieldNullMarkers();
         t.testOSR();
@@ -636,6 +660,9 @@ public static void main(String[] args) {
             Asserts.assertEQ(t.field10.c, (char)i);
             t.field11 = new MyValue13((i % 2) == 0);
             Asserts.assertEQ(t.field11.b, (i % 2) == 0);
+
+            // Test flat (null-free) arrays
+            testFlatArray1(false);
         }
 
         // Trigger deoptimization to check that re-materialization takes the null marker into account
@@ -659,6 +686,8 @@ public static void main(String[] args) {
         MyValue7 val10 = new MyValue7(null);
         MyValue7 val11 = null;
         t.testDeopt5(val8, val9, val10, val11, false);
+
+        testFlatArray1(true);
     }
 }