Skip to content

Commit

Permalink
More tests and more fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@TobiHartmann
TobiHartmann committed Nov 26, 2024
1 parent 5246986 commit 7e2ae9c
Show file tree
Hide file tree
Showing 7 changed files with 220 additions and 30 deletions.
174 changes: 166 additions & 8 deletions Test.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,56 @@ public String toString() {
}
}

@ImplicitlyConstructible
@LooselyConsistentValue
static value class MyValue5_3 {
byte x;

public MyValue5_3(byte x) {
this.x = x;
}
}

@ImplicitlyConstructible
@LooselyConsistentValue
static value class MyValue5_2 {
byte x;
MyValue5_3 val;

public MyValue5_2(byte x, MyValue5_3 val) {
this.x = x;
this.val = val;
}
}

@ImplicitlyConstructible
@LooselyConsistentValue
static value class MyValue5_1 {
byte x;
MyValue5_2 val;

public MyValue5_1(byte x, MyValue5_2 val) {
this.x = x;
this.val = val;
}
}

// Value class with deep nesting of nullable flat fields
@ImplicitlyConstructible
@LooselyConsistentValue
static value class MyValue5 {
byte x;
MyValue5_1 val;

public MyValue5(byte x, MyValue5_1 val) {
this.x = x;
this.val = val;
}
}

MyValue1 field1;
MyValue4 field2;
MyValue5 field3;

// Test that the calling convention is keeping track of the null marker
public MyValue1 testHelper1(MyValue1 val) {
Expand All @@ -104,16 +152,16 @@ public void testDeopt1(byte x, MyValue1 neverNull, MyValue1 alwaysNull, boolean
if (val1.x != x) throw new RuntimeException("FAIL");
if (val1.val1 != val2) throw new RuntimeException("FAIL");
if (val1.val2 != val2) throw new RuntimeException("FAIL");
if (neverNull.x != x) throw new RuntimeException("FAIL");
if (neverNull.val1 != val2) throw new RuntimeException("FAIL");
if (neverNull.x != x) throw new RuntimeException("FAIL");
if (neverNull.val1 != val2) throw new RuntimeException("FAIL");
if (neverNull.val2 != val2) throw new RuntimeException("FAIL");
if (alwaysNull.x != x) throw new RuntimeException("FAIL");
if (alwaysNull.val1 != null) throw new RuntimeException("FAIL");
if (alwaysNull.x != x) throw new RuntimeException("FAIL");
if (alwaysNull.val1 != null) throw new RuntimeException("FAIL");
if (alwaysNull.val2 != null) throw new RuntimeException("FAIL");
}
}

public void testOSR1() {
public void testOSR() {
// Trigger OSR
for (int i = 0; i < 100_000; ++i) {
field1 = null;
Expand Down Expand Up @@ -157,9 +205,62 @@ public void testDeopt2(byte x, MyValue4 neverNull, MyValue4 alwaysNull, boolean
}
}

// Test that the calling convention is keeping track of the null marker
public MyValue5 testHelper3(MyValue5 val) {
return val;
}

public void testSet3(MyValue5 val) {
field3 = testHelper3(val);
}

public MyValue5 testGet3() {
return field3;
}

public void testDeopt3(byte x, MyValue5 val6, MyValue5 val7, MyValue5 val8, MyValue5 val9, boolean deopt) {
MyValue5 val1 = new MyValue5(x, new MyValue5_1(x, new MyValue5_2(x, new MyValue5_3(x))));
MyValue5 val2 = new MyValue5(x, new MyValue5_1(x, new MyValue5_2(x, null)));
MyValue5 val3 = new MyValue5(x, new MyValue5_1(x, null));
MyValue5 val4 = new MyValue5(x, null);
MyValue5 val5 = null;
if (deopt) {
if (val1.x != x) throw new RuntimeException("FAIL");
if (val1.val.x != x) throw new RuntimeException("FAIL");
if (val1.val.val.x != x) throw new RuntimeException("FAIL");
if (val1.val.val.val.x != x) throw new RuntimeException("FAIL");
if (val2.x != x) throw new RuntimeException("FAIL");
if (val2.val.x != x) throw new RuntimeException("FAIL");
if (val2.val.val.x != x) throw new RuntimeException("FAIL");
if (val2.val.val.val != null) throw new RuntimeException("FAIL");
if (val3.x != x) throw new RuntimeException("FAIL");
if (val3.val.x != x) throw new RuntimeException("FAIL");
if (val3.val.val != null) throw new RuntimeException("FAIL");
if (val4.x != x) throw new RuntimeException("FAIL");
if (val4.val != null) throw new RuntimeException("FAIL");
if (val5 != null) throw new RuntimeException("FAIL");

if (val6.x != x) throw new RuntimeException("FAIL");
if (val6.val.x != x) throw new RuntimeException("FAIL");
if (val6.val.val.x != x) throw new RuntimeException("FAIL");
if (val6.val.val.val.x != x) throw new RuntimeException("FAIL");
if (val7.x != x) throw new RuntimeException("FAIL");
if (val7.val.x != x) throw new RuntimeException("FAIL");
if (val7.val.val.x != x) throw new RuntimeException("FAIL");
if (val7.val.val.val != null) throw new RuntimeException("FAIL");
if (val8.x != x) throw new RuntimeException("FAIL");
if (val8.val.x != x) throw new RuntimeException("FAIL");
if (val8.val.val != null) throw new RuntimeException("FAIL");
if (val9.x != x) throw new RuntimeException("FAIL");
if (val9.val != null) throw new RuntimeException("FAIL");
}
}

// TODO add test with empty inline types

public static void main(String[] args) {
Test t = new Test();
t.testOSR1();
t.testOSR();
for (int i = 0; i < 100_000; ++i) {
t.field1 = null;
if (t.testGet1() != null) throw new RuntimeException("FAIL1");
Expand Down Expand Up @@ -205,10 +306,67 @@ public static void main(String[] args) {
if (t.field2.val2 != val3) throw new RuntimeException("FAIL3");

t.testDeopt2((byte)i, null, null, false);

t.field3 = null;
if (t.testGet3() != null) throw new RuntimeException("FAIL1");

boolean useNull_1 = (i % 4) == 0;
boolean useNull_2 = (i % 4) == 1;
boolean useNull_3 = (i % 4) == 2;
MyValue5_3 val5_3 = useNull_3 ? null : new MyValue5_3((byte)i);
MyValue5_2 val5_2 = useNull_2 ? null : new MyValue5_2((byte)i, val5_3);
MyValue5_1 val5_1 = useNull_1 ? null : new MyValue5_1((byte)i, val5_2);
MyValue5 val5 = new MyValue5((byte)i, val5_1);
t.field3 = val5;
if (t.field3.x != val5.x) throw new RuntimeException("FAIL3");
if (useNull_1) {
if (t.field3.val != null) throw new RuntimeException("FAIL3");
} else {
if (t.field3.val.x != val5_1.x) throw new RuntimeException("FAIL3");
if (useNull_2) {
if (t.field3.val.val != null) throw new RuntimeException("FAIL3");
} else {
if (t.field3.val.val.x != val5_2.x) throw new RuntimeException("FAIL3");
if (useNull_3) {
if (t.field3.val.val.val != null) throw new RuntimeException("FAIL3");
} else {
if (t.field3.val.val.val.x != val5_3.x) throw new RuntimeException("FAIL3");
}
}
}

t.testSet3(null);
if (t.field3 != null) throw new RuntimeException("FAIL3");

t.testSet3(val5);
if (t.field3.x != val5.x) throw new RuntimeException("FAIL3");
if (useNull_1) {
if (t.field3.val != null) throw new RuntimeException("FAIL3");
} else {
if (t.field3.val.x != val5_1.x) throw new RuntimeException("FAIL3");
if (useNull_2) {
if (t.field3.val.val != null) throw new RuntimeException("FAIL3");
} else {
if (t.field3.val.val.x != val5_2.x) throw new RuntimeException("FAIL3");
if (useNull_3) {
if (t.field3.val.val.val != null) throw new RuntimeException("FAIL3");
} else {
if (t.field3.val.val.val.x != val5_3.x) throw new RuntimeException("FAIL3");
}
}
}
t.testDeopt3((byte)i, null, null, null, null, false);
}

// Trigger deoptimization to check that re-materialization takes the null marker into account
t.testDeopt1((byte)42, new MyValue1((byte)42, new MyValue2((byte)42), new MyValue2((byte)42)), new MyValue1((byte)42, null, null), true);
t.testDeopt2((byte)42, new MyValue4(new MyValue3((byte)42), new MyValue3((byte)42)), new MyValue4(null, null), true);
byte x = (byte)42;
t.testDeopt1(x, new MyValue1(x, new MyValue2(x), new MyValue2(x)), new MyValue1(x, null, null), true);
t.testDeopt2(x, new MyValue4(new MyValue3(x), new MyValue3(x)), new MyValue4(null, null), true);

MyValue5 val1 = new MyValue5(x, new MyValue5_1(x, new MyValue5_2(x, new MyValue5_3(x))));
MyValue5 val2 = new MyValue5(x, new MyValue5_1(x, new MyValue5_2(x, null)));
MyValue5 val3 = new MyValue5(x, new MyValue5_1(x, null));
MyValue5 val4 = new MyValue5(x, null);
t.testDeopt3(x, val1, val2, val3, val4, true);
}
}
27 changes: 15 additions & 12 deletions src/hotspot/share/oops/inlineKlass.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,49 +294,51 @@ Klass* InlineKlass::value_array_klass_or_null() {
//
// Value classes could also have fields in abstract super value classes.
// Use a HierarchicalFieldStream to get them as well.
int InlineKlass::collect_fields(GrowableArray<SigEntry>* sig, int base_off, int null_marker_offset) {
int InlineKlass::collect_fields(GrowableArray<SigEntry>* sig, float& max_offset, int base_off, int null_marker_offset) {
int count = 0;
SigEntry::add_entry(sig, T_METADATA, name(), base_off);
int max_offset = 0;
max_offset = base_off;
for (HierarchicalFieldStream<JavaFieldStream> fs(this); !fs.done(); fs.next()) {
if (fs.access_flags().is_static()) continue;
int offset = base_off + fs.offset() - (base_off > 0 ? first_field_offset() : 0);
// TODO 8284443 Use different heuristic to decide what should be scalarized in the calling convention
if (fs.is_flat()) {
// Resolve klass of flat field and recursively collect fields
int null_marker_offset = -1;
int field_null_marker_offset = -1;
if (!fs.is_null_free_inline_type()) {
// TODO can we use null_marker_offset() instead?
InlineLayoutInfo* li = inline_layout_info_adr(fs.index());
null_marker_offset = li->null_marker_offset();
field_null_marker_offset = base_off + li->null_marker_offset() - (base_off > 0 ? first_field_offset() : 0);
}
Klass* vk = get_inline_type_field_klass(fs.index());
count += InlineKlass::cast(vk)->collect_fields(sig, offset, null_marker_offset);
count += InlineKlass::cast(vk)->collect_fields(sig, max_offset, offset, field_null_marker_offset);
} else {
BasicType bt = Signature::basic_type(fs.signature());
SigEntry::add_entry(sig, bt, fs.signature(), offset);
count += type2size[bt];
}
max_offset = MAX2(max_offset, offset);
max_offset = MAX2(max_offset, (float)offset);
}
int offset = base_off + size_helper()*HeapWordSize - (base_off > 0 ? first_field_offset() : 0);
// Null markers are no real fields, add them manually at the end (C2 relies on this) of the flat fields
if (null_marker_offset != -1) {
tty->print_cr("MARKER %d %d %d", null_marker_offset, offset, max_offset + 1);
SigEntry::add_entry(sig, T_BOOLEAN, name(), null_marker_offset, max_offset + 1);
max_offset += 0.1f; // We add the markers "in-between" because they are no real fields
tty->print_cr("MARKER %d %d %f", null_marker_offset, offset, max_offset);
SigEntry::add_entry(sig, T_BOOLEAN, name(), null_marker_offset, max_offset);
count++;
}
// TODO I think this is incorrect
SigEntry::add_entry(sig, T_VOID, name(), offset);
if (base_off == 0) {
sig->sort(SigEntry::compare);
}
/*

tty->print_cr("##\n");
print();
for (int i = 0; i < sig->length(); ++i) {
tty->print_cr("%s %d %d", type2name(sig->at(i)._bt), sig->at(i)._offset, sig->at(i)._sort_offset);
tty->print_cr("%s %d %f %s", type2name(sig->at(i)._bt), sig->at(i)._offset, sig->at(i)._sort_offset, (sig->at(i)._offset != sig->at(i)._sort_offset) ? " MARKER" : "");
}
*/

assert(sig->at(0)._bt == T_METADATA && sig->at(sig->length()-1)._bt == T_VOID, "broken structure");
return count;
}
Expand All @@ -348,7 +350,8 @@ void InlineKlass::initialize_calling_convention(TRAPS) {
if (InlineTypeReturnedAsFields || InlineTypePassFieldsAsArgs) {
ResourceMark rm;
GrowableArray<SigEntry> sig_vk;
int nb_fields = collect_fields(&sig_vk);
float max_offset = 0;
int nb_fields = collect_fields(&sig_vk, max_offset);
Array<SigEntry>* extended_sig = MetadataFactory::new_array<SigEntry>(class_loader_data(), sig_vk.length(), CHECK);
*((Array<SigEntry>**)adr_extended_sig()) = extended_sig;
for (int i = 0; i < sig_vk.length(); i++) {
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/oops/inlineKlass.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ class InlineKlass: public InstanceKlass {
virtual void metaspace_pointers_do(MetaspaceClosure* it);

private:
int collect_fields(GrowableArray<SigEntry>* sig, int base_off = 0, int null_marker_offset = -1);
int collect_fields(GrowableArray<SigEntry>* sig, float& max_offset, int base_off = 0, int null_marker_offset = -1);

void cleanup_blobs();

Expand Down
32 changes: 29 additions & 3 deletions src/hotspot/share/opto/inlinetypenode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,32 @@ Node* InlineTypeNode::field_value(uint index) const {
return in(Values + index);
}


// TODO implement with a worklist
/*
static uint helper2(InlineTypeNode* vt, Unique_Node_List& worklist, Node_List& null_markers) {
uint cnt = 0;
for (uint i = 0; i < vt->field_count(); ++i) {
Node* value = vt->field_value(i);
if (vt->field_is_flat(i)) {
cnt += helper(value->as_InlineType(), worklist, null_markers, sfpt);
if (!vt->field_is_null_free(i)) {
vt->field_null_marker_offset(i)
null_markers.push(value->as_InlineType()->get_is_init());
}
} else {
if (value->is_InlineType()) {
// Add inline type field to the worklist to process later
worklist.push(value);
}
sfpt->add_req(value);
cnt++;
}
}
return cnt;
}
*/

// Get the value of the field at the given offset.
// If 'recursive' is true, flat inline type fields will be resolved recursively.
Node* InlineTypeNode::field_value_by_offset(int offset, bool recursive) const {
Expand Down Expand Up @@ -487,7 +513,7 @@ void InlineTypeNode::load(GraphKit* kit, Node* base, Node* ptr, ciInstanceKlass*
} else if (field_is_flat(i)) {
// Recursively load the flat inline type field
assert(field_is_null_free(i) == (field_null_marker_offset(i) == -1), "inconsistency");
value = make_from_flat_impl(kit, ft->as_inline_klass(), base, ptr, holder, offset, field_null_marker_offset(i), decorators, visited);
value = make_from_flat_impl(kit, ft->as_inline_klass(), base, ptr, holder, offset, holder_offset + field_null_marker_offset(i), decorators, visited);
} else {
const TypeOopPtr* oop_ptr = kit->gvn().type(base)->isa_oopptr();
bool is_array = (oop_ptr->isa_aryptr() != nullptr);
Expand Down Expand Up @@ -540,7 +566,7 @@ void InlineTypeNode::store_flat(GraphKit* kit, Node* base, Node* ptr, ciInstance
holder = inline_klass();
}
if (null_marker_offset != -1) {
// Nullable flat field, read the null marker
// Nullable flat field, store the null marker
Node* adr = kit->basic_plus_adr(base, null_marker_offset);
const TypePtr* adr_type = kit->gvn().type(adr)->isa_ptr();
int alias_idx = kit->C->get_alias_index(adr_type);
Expand All @@ -560,7 +586,7 @@ void InlineTypeNode::store(GraphKit* kit, Node* base, Node* ptr, ciInstanceKlass
if (field_is_flat(i)) {
// Recursively store the flat inline type field
assert(field_is_null_free(i) == (field_null_marker_offset(i) == -1), "inconsistency");
value->as_InlineType()->store_flat(kit, base, ptr, holder, offset, field_null_marker_offset(i), decorators);
value->as_InlineType()->store_flat(kit, base, ptr, holder, offset, holder_offset + field_null_marker_offset(i), decorators);
} else {
// Store field value to memory
const TypePtr* adr_type = field_adr_type(base, offset, holder, decorators, kit->gvn());
Expand Down
7 changes: 5 additions & 2 deletions src/hotspot/share/runtime/deoptimization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1514,7 +1514,9 @@ static int compare(ReassignedField* left, ReassignedField* right) {
// 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) {
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>();
}
Expand Down Expand Up @@ -1638,11 +1640,12 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap
}
svIndex++;
}
if (base_offset == 0) {
if (doIt) {
for (int i = 0; i < null_markers->length(); ++i) {
int offset = null_markers->at(i)._offset;
tty->print_cr("Null marker %d %d", offset, svIndex);
jbyte is_init = (jbyte)StackValue::create_stack_value(fr, reg_map, sv->field_at(svIndex++))->get_jint();
tty->print_cr("NULL MARKER %d %d", offset, is_init);
tty->print_cr("Value %d", is_init);
obj->byte_field_put(offset, is_init);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/signature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ ssize_t SignatureVerifier::is_valid_type(const char* type, ssize_t limit) {
#endif // ASSERT

// Adds an argument to the signature
void SigEntry::add_entry(GrowableArray<SigEntry>* sig, BasicType bt, Symbol* symbol, int offset, int sort_offset) {
void SigEntry::add_entry(GrowableArray<SigEntry>* sig, BasicType bt, Symbol* symbol, int offset, float sort_offset) {
if (sort_offset == -1) {
sort_offset = offset;
}
Expand Down
Loading

0 comments on commit 7e2ae9c

Please sign in to comment.