8356963: [lworld] MemNode::optimize_simple_memory_chain fails with "sanity check for basic cases"

Reviewed-by: thartmann

Author: Quan Anh Mai

src/hotspot/share/ci/ciField.cpp

@@ -22,19 +22,24 @@
  *
  */
 
+#include "ci/ciConstant.hpp"
 #include "ci/ciField.hpp"
+#include "ci/ciInlineKlass.hpp"
 #include "ci/ciInstanceKlass.hpp"
+#include "ci/ciSymbol.hpp"
 #include "ci/ciSymbols.hpp"
 #include "ci/ciUtilities.inline.hpp"
 #include "classfile/javaClasses.hpp"
 #include "classfile/vmClasses.hpp"
 #include "gc/shared/collectedHeap.inline.hpp"
 #include "interpreter/linkResolver.hpp"
+#include "jvm_io.h"
 #include "oops/klass.inline.hpp"
 #include "oops/oop.inline.hpp"
 #include "runtime/fieldDescriptor.inline.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/reflection.hpp"
+#include "utilities/globalDefinitions.hpp"
 
 // ciField
 //
@@ -218,28 +223,57 @@ ciField::ciField(fieldDescriptor *fd) :
 
 // Special copy constructor used to flatten inline type fields by
 // copying the fields of the inline type to a new holder klass.
-ciField::ciField(ciField* field, ciInstanceKlass* holder, int offset, bool is_final) {
-  assert(field->holder()->is_inlinetype() || field->holder()->is_abstract(), "should only be used for inline type field flattening");
-  // Set the is_final flag
-  jint final = is_final ? JVM_ACC_FINAL : ~JVM_ACC_FINAL;
-  AccessFlags flags(field->flags().as_int() & final);
-  _flags = ciFlags(flags);
-  _holder = holder;
-  _offset = offset;
-  // Copy remaining fields
-  _name = field->_name;
-  _signature = field->_signature;
-  _type = field->_type;
-  // Trust final flat fields
-  _is_constant = is_final;
-  _known_to_link_with_put = field->_known_to_link_with_put;
-  _known_to_link_with_get = field->_known_to_link_with_get;
-  _constant_value = field->_constant_value;
-  assert(!field->is_flat(), "field must not be flat");
+ciField::ciField(ciField* declared_field, ciField* subfield) {
+  assert(subfield->holder()->is_inlinetype() || subfield->holder()->is_abstract(), "should only be used for inline type field flattening");
+  assert(!subfield->is_flat(), "subfield must not be flat");
+  assert(declared_field->is_flat(), "declared field must be flat");
+
+  _flags = declared_field->flags();
+  _holder = declared_field->holder();
+  _offset = declared_field->offset_in_bytes() + (subfield->offset_in_bytes() - declared_field->type()->as_inline_klass()->payload_offset());
+
+  char buffer[256];
+  jio_snprintf(buffer, sizeof(buffer), "%s.%s", declared_field->name()->as_utf8(), subfield->name()->as_utf8());
+  _name = ciSymbol::make(buffer);
+
+  _signature = subfield->_signature;
+  _type = subfield->_type;
+  _is_constant = false;
+  _known_to_link_with_put = subfield->_known_to_link_with_put;
+  _known_to_link_with_get = subfield->_known_to_link_with_get;
+  _constant_value = ciConstant();
+
+  _is_flat = false;
+  _is_null_free = false;
+  _null_marker_offset = -1;
+  _original_holder = (subfield->_original_holder != nullptr) ? subfield->_original_holder : subfield->_holder;
+}
+
+// Constructor for the ciField of a null marker
+ciField::ciField(ciField* declared_field) {
+  assert(declared_field->is_flat(), "declared field must be flat");
+  assert(!declared_field->is_null_free(), "must have a null marker");
+
+  _flags = declared_field->flags();
+  _holder = declared_field->holder();
+  _offset = declared_field->null_marker_offset();
+
+  char buffer[256];
+  jio_snprintf(buffer, sizeof(buffer), "%s.$nullMarker$", declared_field->name()->as_utf8());
+  _name = ciSymbol::make(buffer);
+
+  _signature = ciSymbols::bool_signature();
+  _type = ciType::make(T_BOOLEAN);
+
+  _is_constant = false;
+  _known_to_link_with_put = nullptr;
+  _known_to_link_with_get = nullptr;
+  _constant_value = ciConstant();
+
   _is_flat = false;
-  _is_null_free = field->_is_null_free;
-  _null_marker_offset = field->_null_marker_offset;
-  _original_holder = (field->_original_holder != nullptr) ? field->_original_holder : field->_holder;
+  _is_null_free = false;
+  _null_marker_offset = -1;
+  _original_holder = nullptr;
 }
 
 static bool trust_final_non_static_fields(ciInstanceKlass* holder) {

src/hotspot/share/ci/ciField.hpp

@@ -62,7 +62,8 @@ class ciField : public ArenaObj {
 
   ciField(ciInstanceKlass* klass, int index, Bytecodes::Code bc);
   ciField(fieldDescriptor* fd);
-  ciField(ciField* field, ciInstanceKlass* holder, int offset, bool is_final);
+  ciField(ciField* declared_field, ciField* sudfield);
+  ciField(ciField* declared_field);
 
   // shared constructor code
   void initialize_from(fieldDescriptor* fd);

src/hotspot/share/ci/ciInstanceKlass.cpp

@@ -525,6 +525,7 @@ void ciInstanceKlass::compute_nonstatic_fields_impl(const GrowableArray<ciField*
       InlineKlass* k = this_klass->get_inline_type_field_klass(fd.index());
       ciInlineKlass* vk = CURRENT_ENV->get_klass(k)->as_inline_klass();
       field_num += vk->nof_nonstatic_fields();
+      field_num += fd.has_null_marker() ? 1 : 0;
     } else {
       field_num++;
     }
@@ -563,15 +564,12 @@ void ciInstanceKlass::compute_nonstatic_fields_impl(const GrowableArray<ciField*
       ciInlineKlass* vk = CURRENT_ENV->get_klass(k)->as_inline_klass();
       // Iterate over fields of the flat inline type and copy them to 'this'
       for (int i = 0; i < vk->nof_nonstatic_fields(); ++i) {
-        ciField* flat_field = vk->nonstatic_field_at(i);
-        // Adjust offset to account for missing oop header
-        int offset = field_offset + (flat_field->offset_in_bytes() - vk->payload_offset());
-        // A flat field can be treated as final if the non-flat
-        // field is declared final or the holder klass is an inline type itself.
-        bool is_final = fd.is_final() || is_inlinetype();
-        ciField* field = new (arena) ciField(flat_field, this, offset, is_final);
         assert(tmp_fields != nullptr, "should be initialized");
-        tmp_fields->append(field);
+        tmp_fields->append(new (arena) ciField(declared_field, vk->nonstatic_field_at(i)));
+      }
+      if (fd.has_null_marker()) {
+        assert(tmp_fields != nullptr, "should be initialized");
+        tmp_fields->append(new (arena) ciField(declared_field));
       }
     } else {
       assert(tmp_fields != nullptr, "should be initialized");

src/hotspot/share/opto/callnode.cpp

@@ -510,16 +510,9 @@ void JVMState::format(PhaseRegAlloc *regalloc, const Node *n, outputStream* st)
           }
         }
         Node* fld_node = mcall->in(first_ind);
-        ciField* cifield;
         if (iklass != nullptr) {
           st->print(" [");
-          if (0 < (uint)iklass->nof_nonstatic_fields()) {
-            cifield = iklass->nonstatic_field_at(0);
-            cifield->print_name_on(st);
-          } else {
-            // Must be a null marker
-            st->print("null marker");
-          }
+          iklass->nonstatic_field_at(0)->print_name_on(st);
           format_helper(regalloc, st, fld_node, ":", 0, &scobjs);
         } else {
           format_helper(regalloc, st, fld_node, "[", 0, &scobjs);
@@ -528,13 +521,7 @@ void JVMState::format(PhaseRegAlloc *regalloc, const Node *n, outputStream* st)
           fld_node = mcall->in(first_ind+j);
           if (iklass != nullptr) {
             st->print(", [");
-            if (j < (uint)iklass->nof_nonstatic_fields()) {
-              cifield = iklass->nonstatic_field_at(j);
-              cifield->print_name_on(st);
-            } else {
-              // Must be a null marker
-              st->print("null marker");
-            }
+            iklass->nonstatic_field_at(j)->print_name_on(st);
             format_helper(regalloc, st, fld_node, ":", j, &scobjs);
           } else {
             format_helper(regalloc, st, fld_node, ", [", j, &scobjs);

src/hotspot/share/opto/inlinetypenode.cpp

@@ -195,59 +195,28 @@ Node* InlineTypeNode::field_value(uint index) const {
   return in(Values + index);
 }
 
-// Get the value of the null marker at the given offset.
-Node* InlineTypeNode::null_marker_by_offset(int offset, int holder_offset) const {
-  // Search through the null markers of all flat fields
-  for (uint i = 0; i < field_count(); ++i) {
-    if (field_is_flat(i)) {
-      InlineTypeNode* value = field_value(i)->as_InlineType();
-      if (!field_is_null_free(i)) {
-        int nm_offset = holder_offset + field_null_marker_offset(i);
-        if (nm_offset == offset) {
-          return value->get_is_init();
-        }
-      }
-      int flat_holder_offset = holder_offset + field_offset(i) - value->inline_klass()->payload_offset();
-      Node* nm_value = value->null_marker_by_offset(offset, flat_holder_offset);
-      if (nm_value != nullptr) {
-        return nm_value;
-      }
-    }
-  }
-  return nullptr;
-}
-
 // 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, bool search_null_marker) const {
-  // First check if we are loading a null marker which is not a real field
-  if (recursive && search_null_marker) {
-    Node* value = null_marker_by_offset(offset);
-    if (value != nullptr){
-      return value;
-    }
-  }
-
-  // If the field at 'offset' belongs to a flat inline type field, 'index' refers to the
-  // corresponding InlineTypeNode input and 'sub_offset' is the offset in the flattened inline type.
+Node* InlineTypeNode::field_value_by_offset(int offset, bool recursive) const {
+  // Find the declared field which contains the field we are looking for
   int index = inline_klass()->field_index_by_offset(offset);
-  int sub_offset = offset - field_offset(index);
   Node* value = field_value(index);
   assert(value != nullptr, "field value not found");
-  if (recursive && value->is_InlineType()) {
-    if (field_is_flat(index)) {
-      // Flat inline type field
-      InlineTypeNode* vt = value->as_InlineType();
-      sub_offset += vt->inline_klass()->payload_offset(); // Add header size
-      return vt->field_value_by_offset(sub_offset, recursive, false);
-    } else {
-      assert(sub_offset == 0, "should not have a sub offset");
-      return value;
-    }
+
+  if (!recursive || !field_is_flat(index)) {
+    assert(offset == field_offset(index), "offset mismatch");
+    return value;
+  }
+
+  // Flat inline type field
+  InlineTypeNode* vt = value->as_InlineType();
+  if (offset == field_null_marker_offset(index)) {
+    return vt->get_is_init();
+  } else {
+    int sub_offset = offset - field_offset(index); // Offset of the flattened field inside the declared field
+    sub_offset += vt->inline_klass()->payload_offset(); // Add header size
+    return vt->field_value_by_offset(sub_offset, recursive);
   }
-  assert(!(recursive && value->is_InlineType()), "should not be an inline type");
-  assert(sub_offset == 0, "offset mismatch");
-  return value;
 }
 
 void InlineTypeNode::set_field_value(uint index, Node* value) {

src/hotspot/share/opto/inlinetypenode.hpp

@@ -110,8 +110,7 @@ class InlineTypeNode : public TypeNode {
   // Inline type fields
   uint          field_count() const { return req() - Values; }
   Node*         field_value(uint index) const;
-  Node*         field_value_by_offset(int offset, bool recursive = false, bool search_null_marker = true) const;
-  Node*         null_marker_by_offset(int offset, int holder_offset = 0) const;
+  Node*         field_value_by_offset(int offset, bool recursive) const;
   void      set_field_value(uint index, Node* value);
   void      set_field_value_by_offset(int offset, Node* value);
   int           field_offset(uint index) const;

src/hotspot/share/opto/memnode.cpp

@@ -153,7 +153,7 @@ static Node* find_call_fallthrough_mem_output(CallNode* call) {
 }
 
 // Try to find a better memory input for a load from a strict final field
-static Node* try_optimize_strict_final_load_memory(PhaseGVN* phase, ciField* field, Node* adr, ProjNode*& base_local) {
+static Node* try_optimize_strict_final_load_memory(PhaseGVN* phase, Node* adr, ProjNode*& base_local) {
   intptr_t offset = 0;
   Node* base = AddPNode::Ideal_base_and_offset(adr, phase, offset);
   if (base == nullptr) {
@@ -246,7 +246,7 @@ Node* MemNode::optimize_simple_memory_chain(Node* mchain, const TypeOopPtr* t_oo
   if (is_strict_final_load) {
     Node* adr = load->in(MemNode::Address);
     assert(phase->type(adr) == t_oop, "inconsistent type");
-    Node* tmp = try_optimize_strict_final_load_memory(phase, field, adr, base_local);
+    Node* tmp = try_optimize_strict_final_load_memory(phase, adr, base_local);
     if (tmp != nullptr) {
       result = tmp;
     }

src/hotspot/share/opto/parse3.cpp

@@ -130,7 +130,7 @@ void Parse::do_get_xxx(Node* obj, ciField* field) {
 
   if (obj->is_InlineType()) {
     InlineTypeNode* vt = obj->as_InlineType();
-    Node* value = vt->field_value_by_offset(field->offset_in_bytes());
+    Node* value = vt->field_value_by_offset(field->offset_in_bytes(), false);
     if (value->is_InlineType()) {
       value = value->as_InlineType()->adjust_scalarization_depth(this);
     }

src/hotspot/share/runtime/signature.hpp

@@ -589,9 +589,12 @@ class SigEntry {
     : _bt(bt), _offset(offset), _sort_offset(sort_offset), _symbol(symbol) {}
 
   static int compare(SigEntry* e1, SigEntry* e2) {
-    if (e1->_sort_offset != e2->_sort_offset) {
-      return e1->_sort_offset - e2->_sort_offset;
+    if (e1->_sort_offset < e2->_sort_offset) {
+      return -1;
+    } else if (e1->_sort_offset > e2->_sort_offset) {
+      return 1;
     }
+
     if (e1->_offset != e2->_offset) {
       return e1->_offset - e2->_offset;
     }

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

@@ -120,7 +120,25 @@ public class InlineTypes {
                          "-XX:+UseFieldFlattening",
                          "-XX:-InlineTypePassFieldsAsArgs",
                          "-XX:-InlineTypeReturnedAsFields"
-            )
+            ),
+            new Scenario(6,
+                         "--enable-preview",
+                         "--add-exports", "java.base/jdk.internal.value=ALL-UNNAMED",
+                         "--add-exports", "java.base/jdk.internal.vm.annotation=ALL-UNNAMED",
+                         "--add-exports", "java.base/jdk.internal.misc=ALL-UNNAMED",
+                         "-XX:+IgnoreUnrecognizedVMOptions",
+                         "-XX:-UseACmpProfile",
+                         "-XX:+AlwaysIncrementalInline",
+                         "-XX:FlatArrayElementMaxOops=5",
+                         "-XX:+UseArrayFlattening",
+                         "-XX:-UseArrayLoadStoreProfile",
+                         "-XX:+UseFieldFlattening",
+                         "-XX:+UseNullableValueFlattening",
+                         "-XX:+UseAtomicValueFlattening",
+                         "-XX:+UseNonAtomicValueFlattening",
+                         "-XX:+InlineTypePassFieldsAsArgs",
+                         "-XX:+InlineTypeReturnedAsFields"
+            ),
     };
 
     public static TestFramework getFramework() {

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

@@ -28,6 +28,8 @@
 import compiler.lib.ir_framework.ForceCompileClassInitializer;
 import compiler.lib.ir_framework.ForceInline;
 
+import java.util.Arrays;
+
 import jdk.internal.vm.annotation.LooselyConsistentValue;
 import jdk.internal.vm.annotation.NullRestricted;
 import jdk.internal.vm.annotation.Strict;
@@ -133,13 +135,6 @@ public long hashInterpreted() {
         return s + sf + x + y + z + o + oa[0] + c + v1.hashInterpreted() + v2.hashInterpreted() + v3.hashInterpreted() + v4.hashInterpreted() + v5.hashInterpreted();
     }
 
-    @Override
-    public String toString() {
-        return "s=" + s + ", sf=" + sf + ", x=" + x + ", y=" + y + ", z=" + z +
-               ", o=" + (o != null ? (Integer)o : "NULL") + ", oa=" + (oa != null ? oa[0] : "NULL") +
-               ", v1=[" + v1 + "], v2=[" + v2 + "], v3=[" + v3 + "], v4=[" + v4 + "], v5=[" + v5 +"], c=" + c;
-    }
-
     @ForceInline
     static MyValue1 setX(MyValue1 v, int x) {
         return new MyValue1(x, v.y, v.z, v.o, v.oa, v.v1, v.v2, v.v4, v.v5, v.c);
@@ -189,4 +184,10 @@ static MyValue1 setV4(MyValue1 v, MyValue2 v4) {
     static MyValue1 setV5(MyValue1 v, MyValue2 v5) {
         return new MyValue1(v.x, v.y, v.z, v.o, v.oa, v.v1, v.v2, v.v4, v5, v.c);
     }
+
+    @Override
+    public String toString() {
+        return "MyValue1[s=" + s + ", sf=" + sf + ", x=" + x + ", y=" + y + ", z=" + z + ", o=" + o + ", oa=" + Arrays.toString(oa) +
+                ", v1=" + v1 + ", v2=" + v2 + ", v4=" + v4 + ", c=" + c + "]";
+    }
 }

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

@@ -66,7 +66,7 @@ public static MyValue2Inline createWithFieldsInline(double d, long l) {
 
     @Override
     public String toString() {
-        return "d=" + d + ", l=" + l;
+        return "MyValue2Inline[d=" + d + ", l=" + l + "]";
     }
 }
 
@@ -129,11 +129,6 @@ public long hashInterpreted() {
         return x + y + (long)v.d + v.l;
     }
 
-    @Override
-    public String toString() {
-        return "x=" + x + ", y=" + y + ", v=[" + v + "]";
-    }
-
     @ForceInline
     static MyValue2 setX(MyValue2 v, int x) {
         return new MyValue2(x, v.y, v.v);
@@ -148,4 +143,9 @@ static MyValue2 setY(MyValue2 v, byte y) {
     static MyValue2 setV(MyValue2 v, MyValue2Inline vi) {
         return new MyValue2(v.x, v.y, vi);
     }
+
+    @Override
+    public String toString() {
+        return "MyValue2[x=" + x + ", y=" + y + ", v=" + v + "]";
+    }
 }