diff --git a/src/hotspot/share/c1/c1_LIRGenerator.cpp b/src/hotspot/share/c1/c1_LIRGenerator.cpp index f2a6be509e5..1e527bc8e4a 100644 --- a/src/hotspot/share/c1/c1_LIRGenerator.cpp +++ b/src/hotspot/share/c1/c1_LIRGenerator.cpp @@ -1656,6 +1656,11 @@ void LIRGenerator::do_StoreField(StoreField* x) { assert(!vk->contains_oops() || !UseZGC, "ZGC does not support embedded oops in flat fields"); #endif + if (!field->is_null_free() && !vk->nullable_atomic_layout_is_natural()) { + bailout("missing support for unnatural nullable atomic layout"); + return; + } + // Zero the payload BasicType bt = vk->atomic_size_to_basic_type(field->is_null_free()); LIR_Opr payload = new_register((bt == T_LONG) ? bt : T_INT); @@ -2103,6 +2108,11 @@ void LIRGenerator::do_LoadField(LoadField* x) { assert(x->state_before() != nullptr, "Needs state before"); #endif + if (!field->is_null_free() && !vk->nullable_atomic_layout_is_natural()) { + bailout("missing support for unnatural nullable atomic layout"); + return; + } + // Allocate buffer (we can't easily do this conditionally on the null check below // because branches added in the LIR are opaque to the register allocator). NewInstance* buffer = new NewInstance(vk, x->state_before(), false, true); diff --git a/src/hotspot/share/ci/ciInlineKlass.cpp b/src/hotspot/share/ci/ciInlineKlass.cpp index 70253d564f7..c759c3a74fe 100644 --- a/src/hotspot/share/ci/ciInlineKlass.cpp +++ b/src/hotspot/share/ci/ciInlineKlass.cpp @@ -106,12 +106,18 @@ int ciInlineKlass::null_marker_offset_in_payload() const { GUARDED_VM_ENTRY(return get_InlineKlass()->null_marker_offset_in_payload();) } +bool ciInlineKlass::nullable_atomic_layout_is_natural() const { + assert(has_nullable_atomic_layout(), "must have the layout to query its nature"); + GUARDED_VM_ENTRY(return get_InlineKlass()->nullable_atomic_layout_is_natural();) +} + // Convert size of atomic layout in bytes to corresponding BasicType BasicType ciInlineKlass::atomic_size_to_basic_type(bool null_free) const { VM_ENTRY_MARK InlineKlass* vk = get_InlineKlass(); assert(!null_free || vk->has_atomic_layout(), "No null-free atomic layout available"); assert( null_free || vk->has_nullable_atomic_layout(), "No nullable atomic layout available"); + assert( null_free || nullable_atomic_layout_is_natural(), "Cannot access the nullable atomic layout naturally"); int size = null_free ? vk->atomic_size_in_bytes() : vk->nullable_atomic_size_in_bytes(); BasicType bt = T_ILLEGAL; if (size == sizeof(jlong)) { diff --git a/src/hotspot/share/ci/ciInlineKlass.hpp b/src/hotspot/share/ci/ciInlineKlass.hpp index 0bc35249889..d4578bec418 100644 --- a/src/hotspot/share/ci/ciInlineKlass.hpp +++ b/src/hotspot/share/ci/ciInlineKlass.hpp @@ -75,6 +75,10 @@ class ciInlineKlass : public ciInstanceKlass { bool has_atomic_layout() const; bool has_nullable_atomic_layout() const; int null_marker_offset_in_payload() const; + + // Whether we can access a nullable atomic field of this type using a single memory instruction. + // Otherwise, we must access the payload and the null marker parts separately. See InlineKlass. + bool nullable_atomic_layout_is_natural() const; BasicType atomic_size_to_basic_type(bool null_free) const; bool must_be_atomic() const; diff --git a/src/hotspot/share/classfile/fieldLayoutBuilder.cpp b/src/hotspot/share/classfile/fieldLayoutBuilder.cpp index e5805c59467..8def0c8f0c1 100644 --- a/src/hotspot/share/classfile/fieldLayoutBuilder.cpp +++ b/src/hotspot/share/classfile/fieldLayoutBuilder.cpp @@ -91,7 +91,7 @@ static void get_size_and_alignment(InlineKlass* vk, LayoutKind kind, int* size, break; case LayoutKind::NULLABLE_ATOMIC_FLAT: *size = vk->nullable_atomic_size_in_bytes(); - *alignment = *size; + *alignment = vk->payload_alignment(); break; default: ShouldNotReachHere(); @@ -1144,11 +1144,13 @@ void FieldLayoutBuilder::compute_inline_class_layout() { _non_atomic_layout_alignment = _payload_alignment; } + int required_alignment = _payload_alignment; // Next step is to compute the characteristics for a layout enabling atomic updates if (UseAtomicValueFlattening) { int atomic_size = _payload_size_in_bytes == 0 ? 0 : round_up_power_of_2(_payload_size_in_bytes); if (atomic_size <= (int)MAX_ATOMIC_OP_SIZE) { _atomic_layout_size_in_bytes = atomic_size; + required_alignment = MAX2(required_alignment, atomic_size); } } @@ -1191,13 +1193,21 @@ void FieldLayoutBuilder::compute_inline_class_layout() { if (nullable_size <= (int)MAX_ATOMIC_OP_SIZE) { _nullable_layout_size_in_bytes = nullable_size; _null_marker_offset = null_marker_offset; + required_alignment = MAX2(required_alignment, nullable_size); } else { - // If the nullable layout is rejected, the NULL_MARKER block should be removed - // from the layout, otherwise it will appear anyway if the layout is printer - if (!_is_empty_inline_class) { // empty values don't have a dedicated NULL_MARKER block - _layout->remove_null_marker(); + if (_atomic_layout_size_in_bytes > 0 && _nonstatic_oopmap_count == 0) { + // Don't do this if the payload has an oop, storing null ignores the payload, which may + // result in the objects there being unnecessarily kept by the GC (a.k.a memory leak) + _nullable_layout_size_in_bytes = _atomic_layout_size_in_bytes + 1; + _null_marker_offset = null_marker_offset; + } else { + // If the nullable layout is rejected, the NULL_MARKER block should be removed + // from the layout, otherwise it will appear anyway if the layout is printer + if (!_is_empty_inline_class) { // empty values don't have a dedicated NULL_MARKER block + _layout->remove_null_marker(); + } + _null_marker_offset = -1; } - _null_marker_offset = -1; } } // If the inline class has an atomic or nullable (which is also atomic) layout, @@ -1207,13 +1217,7 @@ void FieldLayoutBuilder::compute_inline_class_layout() { // doesn't have inherited fields (offsets of inherited fields cannot be changed). If a // field shift is needed but not possible, all atomic layouts are disabled and only reference // and loosely consistent are supported. - int required_alignment = _payload_alignment; - if (has_atomic_layout() && _payload_alignment < atomic_layout_size_in_bytes()) { - required_alignment = atomic_layout_size_in_bytes(); - } - if (has_nullable_atomic_layout() && _payload_alignment < nullable_layout_size_in_bytes()) { - required_alignment = nullable_layout_size_in_bytes(); - } + assert(is_power_of_2(required_alignment), "%s does not have a valid alignment: %d", _classname->as_utf8(), required_alignment); int shift = first_field->offset() % required_alignment; if (shift != 0) { if (required_alignment > _payload_alignment && !_layout->has_inherited_fields()) { diff --git a/src/hotspot/share/oops/inlineKlass.cpp b/src/hotspot/share/oops/inlineKlass.cpp index 4905498bc87..30a5d5711b8 100644 --- a/src/hotspot/share/oops/inlineKlass.cpp +++ b/src/hotspot/share/oops/inlineKlass.cpp @@ -40,17 +40,20 @@ #include "oops/flatArrayKlass.hpp" #include "oops/inlineKlass.inline.hpp" #include "oops/instanceKlass.inline.hpp" +#include "oops/layoutKind.hpp" #include "oops/method.hpp" #include "oops/objArrayKlass.hpp" #include "oops/oop.inline.hpp" #include "oops/refArrayKlass.hpp" #include "runtime/fieldDescriptor.inline.hpp" #include "runtime/handles.inline.hpp" +#include "runtime/orderAccess.hpp" #include "runtime/safepointVerifiers.hpp" #include "runtime/sharedRuntime.hpp" #include "runtime/signature.hpp" #include "runtime/thread.inline.hpp" #include "utilities/copy.hpp" +#include "utilities/powerOfTwo.hpp" #include "utilities/stringUtils.hpp" // Constructor @@ -188,24 +191,37 @@ void InlineKlass::copy_payload_to_addr(void* src, void* dst, LayoutKind lk, bool assert(lk != LayoutKind::REFERENCE && lk != LayoutKind::UNKNOWN, "Sanity check"); switch(lk) { case LayoutKind::NULLABLE_ATOMIC_FLAT: { - if (is_payload_marked_as_null((address)src)) { + if (is_payload_marked_as_null((address)src)) { if (!contains_oops()) { mark_payload_as_null((address)dst); return; } // copy null_reset value to dest + assert(nullable_atomic_layout_is_natural(), "classes with unnatural nullable layout should not contain oops"); if (dest_is_initialized) { HeapAccess<>::value_copy(payload_addr(null_reset_value()), dst, this, lk); } else { HeapAccess::value_copy(payload_addr(null_reset_value()), dst, this, lk); } } else { - // Copy has to be performed, even if this is an empty value, because of the null marker - mark_payload_as_non_null((address)src); - if (dest_is_initialized) { - HeapAccess<>::value_copy(src, dst, this, lk); + if (!nullable_atomic_layout_is_natural()) { + // Copy the payload and the null marker separately + OrderAccess::acquire(); // Acquire between loading the null marker and the payload in src + if (dest_is_initialized) { + HeapAccess<>::value_copy(src, dst, this, LayoutKind::ATOMIC_FLAT); + } else { + HeapAccess::value_copy(src, dst, this, LayoutKind::ATOMIC_FLAT); + } + OrderAccess::release(); // Release between storing the payload and the null marker in dst + mark_payload_as_non_null((address)dst); } else { - HeapAccess::value_copy(src, dst, this, lk); + // Copy has to be performed, even if this is an empty value, because of the null marker + mark_payload_as_non_null((address)src); + if (dest_is_initialized) { + HeapAccess<>::value_copy(src, dst, this, lk); + } else { + HeapAccess::value_copy(src, dst, this, lk); + } } } } diff --git a/src/hotspot/share/oops/inlineKlass.hpp b/src/hotspot/share/oops/inlineKlass.hpp index bdb2849acb8..f04a5e54ca5 100644 --- a/src/hotspot/share/oops/inlineKlass.hpp +++ b/src/hotspot/share/oops/inlineKlass.hpp @@ -31,11 +31,62 @@ #include "oops/instanceKlass.hpp" #include "oops/method.hpp" #include "runtime/registerMap.hpp" +#include "utilities/powerOfTwo.hpp" // An InlineKlass is a specialized InstanceKlass for concrete value classes // (abstract value classes are represented by InstanceKlass) +/** + There are 2 ways to access a nullable atomic field or array element. If the payload including the + null marker fits into a jlong, then we can just access the element as a whole. Otherwise, we can + try another strategy, since the payload is only relevant if the null marker is 1. We can achieve + a field that is accessed as if it is atomic even if the access consists of 2 native accesses. + A store of a not-null Long into a nullable Long field can be executed as: + + store field.value; + release_fence; + store field.null_marker; + + and the store of a null into that field will be: + + store field.null_marker; + + While, a load can be executed as: + + load field.null_marker; + acquire_fence; + load field.value; + + What we need to prove is that, given n concurrent stores, then: + + 1. The final state of the memory must be one of the executed stores: + Consider the stores into the null marker: + - If the last state of the null marker is 0, then the field is null + - If the last state of the null marker is 1, then the field is non-null. In this case, only the + threads that store non-null Long objects touch the memory of value. One of which would be the + last state of the memory here. And it is as if we have a single non-null store that is the + last state. + + Note that the fences are irrelevant for these conditions. + + 2. Given a concurrent load, then it must either observe the initial state, or 1 of the + stores that is executing: + + - If it observes the null marker being 0, then it observes field being null. In this case, only + the null marker is relevant, and it is trivially atomic. + - If it observes the null marker being 1, then it observes field being non-null. In this case, + if the initial state is null, we must observe the null marker being stored by 1 of the + threads. And since we have fences, we must at least observe the value stored by that thread + (or another thread, the point here is that we cannot observe the value in its initial state). + Otherwise, the original state is non-null, we must observe the initial value or one of the + values stored by the threads that try to store non-null. + + As a result, we can see that in any case, the field accesses act as it they are atomic. + + Note that a store of null to a flattened field ignores the payload, so we avoid flattening like + this if the class has oop fields because they can leak. +*/ class InlineKlass: public InstanceKlass { friend class VMStructs; friend class InstanceKlass; @@ -157,6 +208,11 @@ class InlineKlass: public InstanceKlass { int null_marker_offset_in_payload() const { return null_marker_offset() - payload_offset(); } void set_null_marker_offset(int offset) { *(int*)adr_null_marker_offset() = offset; } + bool nullable_atomic_layout_is_natural() const { + assert(has_nullable_atomic_layout(), "must have the layout the query its nature"); + return is_power_of_2(nullable_atomic_size_in_bytes()); + } + bool is_payload_marked_as_null(address payload) { assert(has_nullable_atomic_layout(), " Must have"); return *((jbyte*)payload + null_marker_offset_in_payload()) == 0; diff --git a/src/hotspot/share/opto/inlinetypenode.cpp b/src/hotspot/share/opto/inlinetypenode.cpp index ae2bfa7f561..957e8aa1a04 100644 --- a/src/hotspot/share/opto/inlinetypenode.cpp +++ b/src/hotspot/share/opto/inlinetypenode.cpp @@ -1564,7 +1564,23 @@ void LoadFlatNode::expand_atomic(PhaseIterGVN& igvn) { Node* base = this->base(); Node* ptr = this->ptr(); - BasicType payload_bt = _vk->atomic_size_to_basic_type(_null_free); + bool payload_no_null_marker = _null_free; + if (!_null_free && !_vk->nullable_atomic_layout_is_natural()) { + // Cannot access the whole element in 1 instruction, use 2 memory accesses in a way that seems + // atomic, see InlineKlass + ProjNode* proj_out = proj_out_or_null(TypeFunc::Parms + _vk->nof_nonstatic_fields()); + if (proj_out != nullptr) { + Node* null_marker_ptr = kit.basic_plus_adr(base, ptr, _vk->null_marker_offset_in_payload()); + const TypePtr* null_marker_ptr_type = null_marker_ptr->Value(&igvn)->is_ptr(); + igvn.set_type(null_marker_ptr, null_marker_ptr_type); + Node* null_marker_value = kit.access_load_at(base, null_marker_ptr, null_marker_ptr_type, TypeInt::BOOL, T_BOOLEAN, _decorators); + igvn.replace_node(proj_out, null_marker_value); + kit.insert_mem_bar(Op_MemBarAcquire); + } + payload_no_null_marker = true; + } + + BasicType payload_bt = _vk->atomic_size_to_basic_type(payload_no_null_marker); kit.insert_mem_bar(Op_MemBarCPUOrder); Node* payload = kit.access_load_at(base, ptr, TypeRawPtr::BOTTOM, Type::get_const_basic_type(payload_bt), payload_bt, _decorators | C2_MISMATCHED | C2_CONTROL_DEPENDENT_LOAD | C2_UNKNOWN_CONTROL_LOAD, kit.control()); @@ -1580,7 +1596,7 @@ void LoadFlatNode::expand_atomic(PhaseIterGVN& igvn) { igvn.replace_node(old_mem, new_mem); } - expand_projs_atomic(igvn, kit.control(), payload); + expand_projs_atomic(igvn, kit.control(), payload, payload_no_null_marker); } void LoadFlatNode::collect_field_types(ciInlineKlass* vk, const Type** field_types, int idx, int limit, bool null_free, bool trust_null_free_oop) { @@ -1641,8 +1657,8 @@ InlineTypeNode* LoadFlatNode::collect_projs(GraphKit* kit, ciInlineKlass* vk, in } // Extract the values of the flattened fields from the loaded payload -void LoadFlatNode::expand_projs_atomic(PhaseIterGVN& igvn, Node* ctrl, Node* payload) { - BasicType payload_bt = _vk->atomic_size_to_basic_type(_null_free); +void LoadFlatNode::expand_projs_atomic(PhaseIterGVN& igvn, Node* ctrl, Node* payload, bool payload_no_null_marker) { + BasicType payload_bt = _vk->atomic_size_to_basic_type(payload_no_null_marker); for (int i = 0; i < _vk->nof_nonstatic_fields(); i++) { ProjNode* proj_out = proj_out_or_null(TypeFunc::Parms + i); if (proj_out == nullptr) { @@ -1656,7 +1672,7 @@ void LoadFlatNode::expand_projs_atomic(PhaseIterGVN& igvn, Node* ctrl, Node* pay igvn.replace_node(proj_out, field_value); } - if (!_null_free) { + if (!payload_no_null_marker) { ProjNode* proj_out = proj_out_or_null(TypeFunc::Parms + _vk->nof_nonstatic_fields()); if (proj_out == nullptr) { return; @@ -1670,6 +1686,10 @@ void LoadFlatNode::expand_projs_atomic(PhaseIterGVN& igvn, Node* ctrl, Node* pay Node* LoadFlatNode::get_payload_value(PhaseIterGVN& igvn, Node* ctrl, BasicType payload_bt, Node* payload, const Type* value_type, BasicType value_bt, int offset) { assert((offset + type2aelembytes(value_bt)) <= type2aelembytes(payload_bt), "Value does not fit into payload"); + if (payload_bt == value_bt) { + return payload; + } + Node* value = nullptr; // Shift to the right position in the long value Node* shift_val = igvn.intcon(offset << LogBitsPerByte); @@ -1739,7 +1759,7 @@ bool StoreFlatNode::expand_non_atomic(PhaseIterGVN& igvn) { const TypePtr* field_ptr_type = field_ptr->Value(&igvn)->is_ptr(); igvn.set_type(field_ptr, field_ptr_type); Node* field_value = value->field_value_by_offset(field->offset_in_bytes(), true); - Node* store = kit.access_store_at(base, field_ptr, field_ptr_type, field_value, igvn.type(field_value), field->type()->basic_type(), _decorators); + kit.access_store_at(base, field_ptr, field_ptr_type, field_value, igvn.type(field_value), field->type()->basic_type(), _decorators); } if (!_null_free) { @@ -1747,7 +1767,7 @@ bool StoreFlatNode::expand_non_atomic(PhaseIterGVN& igvn) { const TypePtr* null_marker_ptr_type = null_marker_ptr->Value(&igvn)->is_ptr(); igvn.set_type(null_marker_ptr, null_marker_ptr_type); Node* null_marker_value = value->get_null_marker(); - Node* store = kit.access_store_at(base, null_marker_ptr, null_marker_ptr_type, null_marker_value, TypeInt::BOOL, T_BOOLEAN, _decorators); + kit.access_store_at(base, null_marker_ptr, null_marker_ptr_type, null_marker_value, TypeInt::BOOL, T_BOOLEAN, _decorators); } Node* old_ctrl = proj_out_or_null(TypeFunc::Control); @@ -1775,32 +1795,72 @@ void StoreFlatNode::expand_atomic(PhaseIterGVN& igvn) { Node* base = this->base(); Node* ptr = this->ptr(); InlineTypeNode* value = this->value(); + ciInlineKlass* vk = igvn.type(value)->inline_klass(); - int oop_off_1 = -1; - int oop_off_2 = -1; - Node* payload = convert_to_payload(igvn, kit.control(), value, _null_free, oop_off_1, oop_off_2); + if (!_null_free && !vk->nullable_atomic_layout_is_natural()) { + // Cannot access the whole element in 1 instruction, use 2 memory accesses in a way that seems + // atomic, see InlineKlass + Node* merge = new RegionNode(3); + igvn.set_type(merge, Type::CONTROL); + igvn.record_for_igvn(merge); + Node* mem_phi = new PhiNode(merge, Type::MEMORY, TypePtr::BOTTOM); + igvn.set_type(mem_phi, Type::MEMORY); + igvn.record_for_igvn(mem_phi); + + Node* null_ctl = kit.top(); + Node* notnull_value = kit.null_check_oop(value, &null_ctl); + Node* before_mem = kit.reset_memory(); + merge->init_req(1, null_ctl); + mem_phi->init_req(1, before_mem); + + if (!kit.stopped()) { + kit.set_all_memory(before_mem); + int oop_off_1 = -1; + int oop_off_2 = -1; + Node* payload = convert_to_payload(igvn, kit.control(), value, true, oop_off_1, oop_off_2); + assert(oop_off_1 == -1 && oop_off_2 == -1, "no oop when nullable atomic layout is not natural"); + BasicType payload_bt = vk->atomic_size_to_basic_type(true); + + kit.insert_mem_bar(Op_MemBarCPUOrder); + kit.access_store_at(base, ptr, TypeRawPtr::BOTTOM, payload, Type::get_const_basic_type(payload_bt), payload_bt, _decorators | C2_MISMATCHED, true, value); + kit.insert_mem_bar(Op_MemBarRelease); + merge->init_req(2, kit.control()); + mem_phi->init_req(2, kit.reset_memory()); + } - ciInlineKlass* vk = igvn.type(value)->inline_klass(); - BasicType payload_bt = vk->atomic_size_to_basic_type(_null_free); - kit.insert_mem_bar(Op_MemBarCPUOrder); - if (!UseG1GC || oop_off_1 == -1) { - // No oop fields or no late barrier expansion. Emit an atomic store of the payload and add GC barriers if needed. - assert(oop_off_2 == -1 || !UseG1GC, "sanity"); - // ZGC does not support compressed oops, so only one oop can be in the payload which is written by a "normal" oop store. - assert((oop_off_1 == -1 && oop_off_2 == -1) || !UseZGC, "ZGC does not support embedded oops in flat fields"); - kit.access_store_at(base, ptr, TypeRawPtr::BOTTOM, payload, Type::get_const_basic_type(payload_bt), payload_bt, _decorators | C2_MISMATCHED, true, value); + kit.set_control(merge); + kit.set_all_memory(mem_phi); + Node* null_marker_ptr = kit.basic_plus_adr(base, ptr, vk->null_marker_offset_in_payload()); + const TypePtr* null_marker_ptr_type = null_marker_ptr->Value(&igvn)->is_ptr(); + igvn.set_type(null_marker_ptr, null_marker_ptr_type); + Node* null_marker_value = value->get_null_marker(); + kit.access_store_at(base, null_marker_ptr, null_marker_ptr_type, null_marker_value, TypeInt::BOOL, T_BOOLEAN, _decorators); } else { - // Contains oops and requires late barrier expansion. Emit a special store node that allows to emit GC barriers in the backend. - assert(UseG1GC, "Unexpected GC"); - assert(payload_bt == T_LONG, "Unexpected payload type"); - // If one oop, set the offset (if no offset is set, two oops are assumed by the backend) - Node* oop_offset = (oop_off_2 == -1) ? igvn.intcon(oop_off_1) : nullptr; - Node* mem = kit.reset_memory(); - kit.set_all_memory(mem); - Node* store = igvn.transform(new StoreLSpecialNode(kit.control(), mem, ptr, TypeRawPtr::BOTTOM, payload, oop_offset, MemNode::unordered)); - kit.set_memory(store, TypeRawPtr::BOTTOM); + int oop_off_1 = -1; + int oop_off_2 = -1; + Node* payload = convert_to_payload(igvn, kit.control(), value, _null_free, oop_off_1, oop_off_2); + + BasicType payload_bt = vk->atomic_size_to_basic_type(_null_free); + kit.insert_mem_bar(Op_MemBarCPUOrder); + if (!UseG1GC || oop_off_1 == -1) { + // No oop fields or no late barrier expansion. Emit an atomic store of the payload and add GC barriers if needed. + assert(oop_off_2 == -1 || !UseG1GC, "sanity"); + // ZGC does not support compressed oops, so only one oop can be in the payload which is written by a "normal" oop store. + assert((oop_off_1 == -1 && oop_off_2 == -1) || !UseZGC, "ZGC does not support embedded oops in flat fields"); + kit.access_store_at(base, ptr, TypeRawPtr::BOTTOM, payload, Type::get_const_basic_type(payload_bt), payload_bt, _decorators | C2_MISMATCHED, true, value); + } else { + // Contains oops and requires late barrier expansion. Emit a special store node that allows to emit GC barriers in the backend. + assert(UseG1GC, "Unexpected GC"); + assert(payload_bt == T_LONG, "Unexpected payload type"); + // If one oop, set the offset (if no offset is set, two oops are assumed by the backend) + Node* oop_offset = (oop_off_2 == -1) ? igvn.intcon(oop_off_1) : nullptr; + Node* mem = kit.reset_memory(); + kit.set_all_memory(mem); + Node* store = igvn.transform(new StoreLSpecialNode(kit.control(), mem, ptr, TypeRawPtr::BOTTOM, payload, oop_offset, MemNode::unordered)); + kit.set_memory(store, TypeRawPtr::BOTTOM); + } + kit.insert_mem_bar(Op_MemBarCPUOrder); } - kit.insert_mem_bar(Op_MemBarCPUOrder); Node* old_ctrl = proj_out_or_null(TypeFunc::Control); if (old_ctrl != nullptr) { @@ -1858,6 +1918,9 @@ Node* StoreFlatNode::convert_to_payload(PhaseIterGVN& igvn, Node* ctrl, InlineTy Node* StoreFlatNode::set_payload_value(PhaseIterGVN& igvn, BasicType payload_bt, Node* payload, BasicType val_bt, Node* value, int offset) { assert((offset + type2aelembytes(val_bt)) <= type2aelembytes(payload_bt), "Value does not fit into payload"); + if (payload_bt == val_bt) { + return value; + } // Make sure to zero unused bits in the 32-bit value if (val_bt == T_BYTE || val_bt == T_BOOLEAN) { diff --git a/src/hotspot/share/opto/inlinetypenode.hpp b/src/hotspot/share/opto/inlinetypenode.hpp index 308433e29a1..95273e06e0a 100644 --- a/src/hotspot/share/opto/inlinetypenode.hpp +++ b/src/hotspot/share/opto/inlinetypenode.hpp @@ -208,7 +208,7 @@ class LoadFlatNode final : public SafePointNode { static void collect_field_types(ciInlineKlass* vk, const Type** field_types, int idx, int limit, bool null_free, bool trust_null_free_oop); InlineTypeNode* collect_projs(GraphKit* kit, ciInlineKlass* vk, int proj_con, bool null_free); - void expand_projs_atomic(PhaseIterGVN& gvn, Node* ctrl, Node* payload); + void expand_projs_atomic(PhaseIterGVN& gvn, Node* ctrl, Node* payload, bool payload_no_null_marker); static Node* get_payload_value(PhaseIterGVN& igvn, Node* ctrl, BasicType payload_bt, Node* payload, const Type* value_type, BasicType value_bt, int offset); }; diff --git a/src/hotspot/share/opto/library_call.cpp b/src/hotspot/share/opto/library_call.cpp index d23f7fb2adf..524ed1b065d 100644 --- a/src/hotspot/share/opto/library_call.cpp +++ b/src/hotspot/share/opto/library_call.cpp @@ -2908,7 +2908,7 @@ bool LibraryCallKit::inline_unsafe_flat_access(bool is_store, AccessKind kind) { Node* value = argument(6); const Type* value_type = _gvn.type(value); if (!value_type->is_inlinetypeptr()) { - value_type = Type::get_const_type(value_klass)->filter_speculative(value_type); + value_type = Type::get_const_type(value_klass); Node* new_value = _gvn.transform(new CastPPNode(control(), value, value_type, ConstraintCastNode::StrongDependency)); new_value = InlineTypeNode::make_from_oop(this, new_value, value_klass); replace_in_map(value, new_value); diff --git a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java index 9c6ea0119e9..1627c387b24 100644 --- a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java +++ b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java @@ -1326,4 +1326,37 @@ public void test48_verifier() { MyValue47Holder v = new MyValue47Holder(rI); Asserts.assertEQ(test48(), v); } + + // Verify that a nullable j.l.Long can be flattened + static class LongHolder { + Long v; + } + + @Test + @IR(applyIfAnd = {"UseFieldFlattening", "true", "UseAtomicValueFlattening", "true"}, counts = {IRNode.STORE_L, "1", IRNode.STORE_B, "1"}) + @IR(applyIfAnd = {"UseFieldFlattening", "true", "UseAtomicValueFlattening", "true"}, failOn = {IRNode.ALLOC}) + public void test49(LongHolder h, long v) { + h.v = v; + } + + @Run(test = "test49") + public void test49_verifier() { + LongHolder h = new LongHolder(); + test49(h, rL); + Asserts.assertEQ(rL, h.v); + } + + @Test + @IR(applyIfAnd = {"UseArrayFlattening", "true", "UseAtomicValueFlattening", "true"}, counts = {IRNode.STORE_L, "1", IRNode.STORE_B, "1"}) + @IR(applyIfAnd = {"UseArrayFlattening", "true", "UseAtomicValueFlattening", "true"}, failOn = {IRNode.ALLOC}) + public void test50(Long[] a, long v) { + a[0] = v; + } + + @Run(test = "test50") + public void test50_verifier() { + Long[] a = new Long[1]; + test50(a, rL); + Asserts.assertEQ(rL, a[0]); + } } diff --git a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestTearing.java b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestTearing.java index accd41bb54b..c8716577c16 100644 --- a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestTearing.java +++ b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestTearing.java @@ -246,6 +246,19 @@ MyValueTearing incrementAndCheckUnsafe() { } } +/** + * A class such that we cannot access its nullable layout with a single + * instruction. + */ +value record MyLargeValueTearing(int x, int y) { + static final MyLargeValueTearing DEFAULT = new MyLargeValueTearing(0, 0); + + MyLargeValueTearing incrementAndCheck() { + Asserts.assertEQ(x, y, "Inconsistent field values"); + return new MyLargeValueTearing(x + 1, y + 1); + } +} + public class TestTearing { private static final WhiteBox WHITE_BOX = WhiteBox.getWhiteBox(); private static final boolean SLOW_CONFIGURATION = @@ -267,6 +280,8 @@ public class TestTearing { static MyValueTearing field3 = new MyValueTearing((short)0, (short)0); MyValueTearing field4 = new MyValueTearing((short)0, (short)0); + MyLargeValueTearing field5 = MyLargeValueTearing.DEFAULT; + // Final arrays static final MyValueTearing[] array1 = (MyValueTearing[])ValueClass.newNullRestrictedAtomicArray(MyValueTearing.class, 1, MyValueTearing.DEFAULT); static final MyValueTearing[] array2 = (MyValueTearing[])ValueClass.newNullableAtomicArray(MyValueTearing.class, 1); @@ -299,7 +314,17 @@ public class TestTearing { } static volatile Object[] array12 = new MyValueTearing[] { new MyValueTearing((short)0, (short)0) }; + static MyLargeValueTearing[] array13 = new MyLargeValueTearing[] { MyLargeValueTearing.DEFAULT }; + + static final Unsafe U = Unsafe.getUnsafe(); static final MethodHandle incrementAndCheck_mh; + static final Field field5_mirror; + static final long field5_offset; + static final int field5_layout; + static final VarHandle field5_vh; + static final long array13_baseOffset; + static final int array13_layout; + static final VarHandle array13_vh; static { try { @@ -308,7 +333,15 @@ public class TestTearing { MethodType mt = MethodType.methodType(MyValueTearing.class); incrementAndCheck_mh = lookup.findVirtual(clazz, "incrementAndCheck", mt); - } catch (NoSuchMethodException | IllegalAccessException e) { + + field5_mirror = TestTearing.class.getDeclaredField("field5"); + field5_offset = U.objectFieldOffset(field5_mirror); + field5_layout = U.fieldLayout(field5_mirror); + field5_vh = lookup.findVarHandle(TestTearing.class, "field5", MyLargeValueTearing.class).withInvokeExactBehavior(); + array13_baseOffset = U.arrayInstanceBaseOffset(array13); + array13_layout = U.arrayLayout(array13); + array13_vh = MethodHandles.arrayElementVarHandle(array13.getClass()).withInvokeExactBehavior(); + } catch (NoSuchFieldException | NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); throw new RuntimeException("Method handle lookup failed"); } @@ -351,6 +384,20 @@ public void run() { test.field2 = test.field2.incrementAndCheck(); test.field3 = test.field3.incrementAndCheck(); test.field4 = test.field4.incrementAndCheck(); + + // Occasionally store a null + MyLargeValueTearing field5Value = test.field5; + if (field5Value == null) { + field5Value = MyLargeValueTearing.DEFAULT; + } else { + field5Value = field5Value.incrementAndCheck(); + } + if ((i & 15) == 0) { + test.field5 = null; + } else { + test.field5 = field5Value; + } + array1[0] = array1[0].incrementAndCheck(); array2[0] = array2[0].incrementAndCheck(); array3[0] = array3[0].incrementAndCheck(); @@ -364,10 +411,38 @@ public void run() { array11[0] = ((MyValueTearing)array11[0]).incrementAndCheck(); array12[0] = ((MyValueTearing)array12[0]).incrementAndCheck(); + // Occasionally store a null + MyLargeValueTearing array13Elem = array13[0]; + if (array13Elem == null) { + array13Elem = MyLargeValueTearing.DEFAULT; + } else { + array13Elem = array13Elem.incrementAndCheck(); + } + if ((i & 15) == 0) { + array13[0] = null; + } else { + array13[0] = array13Elem; + } + test.field1 = test.field1.incrementAndCheckUnsafe(); test.field2 = test.field2.incrementAndCheckUnsafe(); test.field3 = test.field3.incrementAndCheckUnsafe(); test.field4 = test.field4.incrementAndCheckUnsafe(); + + if (field5_layout != 0) { + field5Value = U.getFlatValue(test, field5_offset, field5_layout, MyLargeValueTearing.class); + if (field5Value == null) { + field5Value = MyLargeValueTearing.DEFAULT; + } else { + field5Value = field5Value.incrementAndCheck(); + } + if ((i & 15) == 0) { + U.putFlatValue(test, field5_offset, field5_layout, MyLargeValueTearing.class, null); + } else { + U.putFlatValue(test, field5_offset, field5_layout, MyLargeValueTearing.class, field5Value); + } + } + array1[0] = array1[0].incrementAndCheckUnsafe(); array2[0] = array2[0].incrementAndCheckUnsafe(); array3[0] = array3[0].incrementAndCheckUnsafe(); @@ -381,11 +456,38 @@ public void run() { array11[0] = ((MyValueTearing)array11[0]).incrementAndCheckUnsafe(); array12[0] = ((MyValueTearing)array12[0]).incrementAndCheckUnsafe(); + if (array13_layout != 0) { + array13Elem = U.getFlatValue(array13, array13_baseOffset, array13_layout, MyLargeValueTearing.class); + if (array13Elem == null) { + array13Elem = MyLargeValueTearing.DEFAULT; + } else { + array13Elem = array13Elem.incrementAndCheck(); + } + if ((i & 15) == 0) { + U.putFlatValue(array13, array13_baseOffset, array13_layout, MyLargeValueTearing.class, null); + } else { + U.putFlatValue(array13, array13_baseOffset, array13_layout, MyLargeValueTearing.class, array13Elem); + } + } + try { test.field1 = (MyValueTearing)incrementAndCheck_mh.invokeExact(test.field1); test.field2 = (MyValueTearing)incrementAndCheck_mh.invokeExact(test.field2); test.field3 = (MyValueTearing)incrementAndCheck_mh.invokeExact(test.field1); test.field4 = (MyValueTearing)incrementAndCheck_mh.invokeExact(test.field2); + + field5Value = (MyLargeValueTearing) field5_vh.get(test); + if (field5Value == null) { + field5Value = MyLargeValueTearing.DEFAULT; + } else { + field5Value = field5Value.incrementAndCheck(); + } + if ((i & 15) == 0) { + field5_vh.set(test, (MyLargeValueTearing) null); + } else { + field5_vh.set(test, field5Value); + } + array1[0] = (MyValueTearing)incrementAndCheck_mh.invokeExact(array1[0]); array2[0] = (MyValueTearing)incrementAndCheck_mh.invokeExact(array2[0]); array3[0] = (MyValueTearing)incrementAndCheck_mh.invokeExact(array3[0]); @@ -398,6 +500,18 @@ public void run() { array10[0] = (MyValueTearing)incrementAndCheck_mh.invokeExact((MyValueTearing)array10[0]); array11[0] = (MyValueTearing)incrementAndCheck_mh.invokeExact((MyValueTearing)array11[0]); array12[0] = (MyValueTearing)incrementAndCheck_mh.invokeExact((MyValueTearing)array12[0]); + + array13Elem = (MyLargeValueTearing) array13_vh.get(array13, 0); + if (array13Elem == null) { + array13Elem = MyLargeValueTearing.DEFAULT; + } else { + array13Elem = array13Elem.incrementAndCheck(); + } + if ((i & 15) == 0) { + array13_vh.set(array13, 0, (MyLargeValueTearing) null); + } else { + array13_vh.set(array13, 0, array13Elem); + } } catch (Throwable t) { throw new RuntimeException("Test failed", t); }