From 496f2b86956b5482503b17267d35fad7610cb429 Mon Sep 17 00:00:00 2001 From: Tobias Hartmann Date: Thu, 12 Dec 2024 14:56:03 +0100 Subject: [PATCH] Refactoring and a fix --- src/hotspot/share/opto/inlinetypenode.cpp | 27 +++++++------- src/hotspot/share/opto/macro.cpp | 8 +++-- src/hotspot/share/opto/type.cpp | 2 -- src/hotspot/share/runtime/deoptimization.cpp | 35 +++++++------------ .../inlinetypes/TestFieldNullMarkers.java | 31 +++++++++++++++- 5 files changed, 60 insertions(+), 43 deletions(-) diff --git a/src/hotspot/share/opto/inlinetypenode.cpp b/src/hotspot/share/opto/inlinetypenode.cpp index b10f1040f05..cd6ea2e50b8 100644 --- 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 index f2619f71b5c..c1a40153555 100644 --- 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 index 32b1851ca15..22aaf2c5283 100644 --- 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 index 12b6461f9df..660206c1d10 100644 --- 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* 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* null_marker_offsets, TRAPS) { GrowableArray* fields = new GrowableArray(); - bool doIt = false; - if (null_markers == nullptr) { - doIt = true; - // Null markers are no real fields.. - null_markers = new GrowableArray(); - } 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,11 +1526,6 @@ 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); } @@ -1545,6 +1533,12 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap 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(); + } 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 index cccf98c674b..44dd93ca8ad 100644 --- 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); } }