Merge pull request #16294 from a7ehuo/fix-field-offset-flattened-arra…

…yelement Fix field offset in flattened array element access
eclipse-openj9 · Nov 14, 2022 · c97bd6c · c97bd6c
2 parents d4aba17 + 3a216bd
commit c97bd6c
Show file tree

Hide file tree

Showing 5 changed files with 155 additions and 32 deletions.
diff --git a/runtime/compiler/compile/J9SymbolReferenceTable.cpp b/runtime/compiler/compile/J9SymbolReferenceTable.cpp
@@ -875,16 +875,14 @@ TR::SymbolReference *
 J9::SymbolReferenceTable::findOrFabricateFlattenedArrayElementFieldShadowSymbol(
    TR_OpaqueClassBlock *arrayComponentClass,
    TR::DataType type,
-   uint32_t fieldOffset,
+   int32_t fieldOffset,
    bool isPrivate,
    const char *fieldName,
    const char *fieldSignature)
    {
-   int32_t flattenedFieldOffset = (int32_t)fieldOffset - (int32_t)TR::Compiler->om.objectHeaderSizeInBytes();
+   TR_ASSERT_FATAL(fieldOffset >= 0, "fieldOffset %d is invalid: fieldOffset %u objectHeaderSizeInBytes %" OMR_PRIuPTR " \n", fieldOffset, fieldOffset, TR::Compiler->om.objectHeaderSizeInBytes());
 
-   TR_ASSERT_FATAL(flattenedFieldOffset >= 0, "flattenedFieldOffset %d is invalid: fieldOffset %u objectHeaderSizeInBytes %" OMR_PRIuPTR " \n", flattenedFieldOffset, fieldOffset, TR::Compiler->om.objectHeaderSizeInBytes());
-
-   ResolvedFieldShadowKey key(arrayComponentClass, flattenedFieldOffset, type);
+   ResolvedFieldShadowKey key(arrayComponentClass, fieldOffset, type);
 
    TR::SymbolReference *symRef = findFlattenedArrayElementFieldShadow(key, isPrivate);
    if (symRef != NULL)
@@ -924,7 +922,7 @@ J9::SymbolReferenceTable::findOrFabricateFlattenedArrayElementFieldShadowSymbol(
 
    bool isResolved = true;
    bool isUnresolvedInCP = false;
-   initShadowSymbol(NULL, symRef, isResolved, type, flattenedFieldOffset, isUnresolvedInCP);
+   initShadowSymbol(NULL, symRef, isResolved, type, fieldOffset, isUnresolvedInCP);
 
    _flattenedArrayElementFieldShadows.insert(std::make_pair(key, symRef));
    return symRef;

diff --git a/runtime/compiler/compile/J9SymbolReferenceTable.hpp b/runtime/compiler/compile/J9SymbolReferenceTable.hpp
@@ -209,7 +209,7 @@ class SymbolReferenceTable : public OMR::SymbolReferenceTableConnector
     *  \param type
     *     The data type of the field.
     *  \param fieldOffset
-    *     The offset of the field.
+    *     The offset of the field from the beginning of the first field.
     *  \param isPrivate
     *     Specifies whether the field is private.
     *  \param fieldName
@@ -219,7 +219,7 @@ class SymbolReferenceTable : public OMR::SymbolReferenceTableConnector
     *  \return
     *     Returns an array shadow symbol reference fabricated for the field of a flattened array element.
     */
-   TR::SymbolReference * findOrFabricateFlattenedArrayElementFieldShadowSymbol(TR_OpaqueClassBlock *arrayComponentClass, TR::DataType type, uint32_t fieldOffset, bool isPrivate, const char *fieldName, const char *fieldSignature);
+   TR::SymbolReference * findOrFabricateFlattenedArrayElementFieldShadowSymbol(TR_OpaqueClassBlock *arrayComponentClass, TR::DataType type, int32_t fieldOffset, bool isPrivate, const char *fieldName, const char *fieldSignature);
 
    /** \brief
     *     Returns a symbol reference for default value instance of value class.

diff --git a/runtime/compiler/optimizer/J9ValuePropagation.cpp b/runtime/compiler/optimizer/J9ValuePropagation.cpp
@@ -882,13 +882,17 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node)
             else // isStoreFlattenableArrayElement
                {
                TR_OpaqueClassBlock *valueClass = storeValueConstraint->getClass();
-               TR_YesNoMaybe isInstanceOfComponentType = comp()->fej9()->isInstanceOf(valueClass,
+               // storeValueConstraint might not be a resolved class constraint. Therefore valueClass might not exist
+               if (valueClass)
+                  {
+                  TR_YesNoMaybe isInstanceOfComponentType = comp()->fej9()->isInstanceOf(valueClass,
                                                                                    arrayComponentClass,
                                                                                    storeValueConstraint->isFixedClass(),
                                                                                    arrayConstraint->isFixedClass());
 
-               // If the value that's being stored is not the same as the array component type, use helper call.
-               canTransformFlattenedArrayElementLoadStore = (isInstanceOfComponentType == TR_yes);
+                  // If the value that's being stored is not the same as the array component type, use helper call.
+                  canTransformFlattenedArrayElementLoadStore = (isInstanceOfComponentType == TR_yes);
+                  }
                }
             }
          }
@@ -913,6 +917,10 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node)
          canTransformIdentityLoadStore = true;
          }
 
+      if (trace())
+         traceMsg(comp(), "%s: n%dn %s canTransformFlattenedArrayElementLoadStore %d canTransformUnflattenedVTArrayElementLoadStore %d canTransformIdentityLoadStore %d\n", __FUNCTION__,
+            node->getGlobalIndex(), node->getOpCode().getName(), canTransformFlattenedArrayElementLoadStore, canTransformUnflattenedVTArrayElementLoadStore, canTransformIdentityLoadStore);
+
       if (canTransformFlattenedArrayElementLoadStore
           || canTransformUnflattenedVTArrayElementLoadStore
           || canTransformIdentityLoadStore)
@@ -1957,6 +1965,7 @@ J9::ValuePropagation::transformFlattenedArrayElementLoad(TR_OpaqueClassBlock *ar
    int32_t elementStride = TR::Compiler->cls.flattenedArrayElementSize(comp(), arrayClass);
    TR::Node *elementAddressNode = J9::TransformUtil::calculateElementAddressWithElementStride(comp(), arrayRefNode, indexNode, elementStride);
 
+   int32_t offsetOfFirstField = fieldTypeLayout->entry(0)._offset;
    // Generate load for each of the final fields
    int newValueNodeChildIndex = 1;
    for (size_t idx = 0; idx < fieldCount; idx++)
@@ -1966,7 +1975,7 @@ J9::ValuePropagation::transformFlattenedArrayElementLoad(TR_OpaqueClassBlock *ar
 
       auto * fieldSymRef = comp()->getSymRefTab()->findOrFabricateFlattenedArrayElementFieldShadowSymbol(arrayComponentClass,
                                                                   fieldEntry._datatype,
-                                                                  fieldEntry._offset,
+                                                                  fieldEntry._offset - offsetOfFirstField,
                                                                   fieldEntry._isPrivate,
                                                                   fieldEntry._fieldname,
                                                                   fieldEntry._typeSignature);
@@ -2066,6 +2075,7 @@ J9::ValuePropagation::transformFlattenedArrayElementStore(TR_OpaqueClassBlock *a
    int32_t elementStride = TR::Compiler->cls.flattenedArrayElementSize(comp(), arrayClass);
    TR::Node *elementAddressNode = J9::TransformUtil::calculateElementAddressWithElementStride(comp(), arrayRefNode, indexNode, elementStride);
 
+   int32_t offsetOfFirstField = fieldTypeLayout->entry(0)._offset;
    // Generate store for each of the final fields
    TR::TreeTop *tt = callTree;
    for (size_t idx = 0; idx < fieldCount; idx++)
@@ -2084,7 +2094,7 @@ J9::ValuePropagation::transformFlattenedArrayElementStore(TR_OpaqueClassBlock *a
 
       auto * storeFieldSymRef = comp()->getSymRefTab()->findOrFabricateFlattenedArrayElementFieldShadowSymbol(arrayComponentClass,
                                                                   fieldEntry._datatype,
-                                                                  fieldEntry._offset,
+                                                                  fieldEntry._offset - offsetOfFirstField,
                                                                   fieldEntry._isPrivate,
                                                                   fieldEntry._fieldname,
                                                                   fieldEntry._typeSignature);

diff --git a/test/functional/Valhalla/playlist.xml b/test/functional/Valhalla/playlist.xml
@@ -25,10 +25,10 @@
 		<testCaseName>ValueTypeTests</testCaseName>
 		<variations>
 			<variation>-Xgcpolicy:optthruput</variation>
-			<variation> -XX:ValueTypeFlatteningThreshold=99999 -Xgcpolicy:optthruput -XX:-EnableArrayFlattening</variation>
+			<variation>-Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
 			<variation>-Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
 			<variation>-Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
-			<!-- Use -XX:-EnableArrayFlattening. testDefaultValueInTriangleArray asserts the fields of each element are not NULL, 
+			<!-- Use -XX:-EnableArrayFlattening. testDefaultValueInTriangleArray asserts the fields of each element are not NULL,
 					which is not true as a FlatteningThreshold is set. -->
 			<variation>-Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=12 -XX:-EnableArrayFlattening</variation>
 			<variation>-Xnocompressedrefs -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
@@ -60,17 +60,17 @@
 	</test>
 	<test>
 		<testCaseName>ValueTypeTestsJIT</testCaseName>
-		<disables>
-			<disable>
-				<comment>https://github.com/eclipse-openj9/openj9/pull/9392#issuecomment-661246648</comment>
-				<variation>-Xjit:count=0</variation>
-			</disable>
-		</disables>
 		<variations>
 			<variation>-Xjit:count=0</variation>
 			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput</variation>
-			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999</variation>
-			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=12 -XX:-EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xnocompressedrefs -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xnocompressedrefs -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xnocompressedrefs -Xgcpolicy:gencon</variation>
 		</variations>
 		<command>$(JAVA_COMMAND) $(JVM_OPTIONS) \
 		--add-opens java.base/jdk.internal.misc=ALL-UNNAMED \
@@ -98,7 +98,7 @@
 		<testCaseName>ValueTypeArrayTests</testCaseName>
 		<variations>
 			<variation>-Xgcpolicy:optthruput</variation>
-			<variation> -XX:ValueTypeFlatteningThreshold=99999 -Xgcpolicy:optthruput -XX:-EnableArrayFlattening</variation>
+			<variation>-Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
 			<variation>-Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
 			<variation>-Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
 			<!-- Use -XX:-EnableArrayFlattening. testDefaultValueInTriangleArray asserts the fields of each element are not NULL,
@@ -133,17 +133,17 @@
 	</test>
 	<test>
 		<testCaseName>ValueTypeArrayTestsJIT</testCaseName>
-		<disables>
-			<disable>
-				<comment>https://github.com/eclipse-openj9/openj9/pull/9392#issuecomment-661246648</comment>
-				<variation>-Xjit:count=0</variation>
-			</disable>
-		</disables>
 		<variations>
 			<variation>-Xjit:count=0</variation>
 			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput</variation>
-			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999</variation>
-			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=12 -XX:-EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xnocompressedrefs -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xnocompressedrefs -Xgcpolicy:gencon -XX:ValueTypeFlatteningThreshold=99999 -XX:+EnableArrayFlattening</variation>
+			<variation>-Xjit:count=1,disableAsyncCompilation -Xnocompressedrefs -Xgcpolicy:gencon</variation>
 		</variations>
 		<command>$(JAVA_COMMAND) $(JVM_OPTIONS) \
 		--add-opens java.base/jdk.internal.misc=ALL-UNNAMED \

diff --git a/test/functional/Valhalla/src/org/openj9/test/lworld/ValueTypeArrayTests.java b/test/functional/Valhalla/src/org/openj9/test/lworld/ValueTypeArrayTests.java
@@ -356,4 +356,119 @@ static public void testValueTypeArrayAssignments() throws Throwable {
 			}
 		}
 	}
+
+   static primitive class SomeClassWithDoubleField{
+      public double d;
+
+		SomeClassWithDoubleField(double x) {
+			this.d = x;
+		}
+   }
+
+   static primitive class SomeClassWithFloatField{
+      public float f;
+
+		SomeClassWithFloatField(float x) {
+			this.f = x;
+		}
+   }
+
+   static primitive class SomeClassWithLongField{
+      public long l;
+
+		SomeClassWithLongField(long x) {
+			this.l = x;
+		}
+   }
+
+   static void readArrayElementWithDoubleField(SomeClassWithDoubleField[] data) throws Throwable {
+      for (int i=0; i<data.length; ++i) {
+         assertEquals(data[i].d, (double)i);
+      }
+   }
+
+   static void readArrayElementWithFloatField(SomeClassWithFloatField[] data) throws Throwable {
+      for (int i=0; i<data.length; ++i) {
+         assertEquals(data[i].f, (float)i);
+      }
+   }
+
+   static void readArrayElementWithLongField(SomeClassWithLongField[] data) throws Throwable {
+      for (int i=0; i<data.length; ++i) {
+         assertEquals(data[i].l, (long)i);
+      }
+   }
+
+   static void writeArrayElementWithDoubleField(SomeClassWithDoubleField[] srcData, SomeClassWithDoubleField[] dstData) throws Throwable {
+      for (int i=0; i<dstData.length; ++i) {
+         dstData[i] = srcData[i];
+      }
+
+      for (int i=0; i<dstData.length; ++i) {
+         assertEquals(dstData[i].d, (double)(i+1));
+      }
+   }
+
+   static void writeArrayElementWithFloatField(SomeClassWithFloatField[] srcData, SomeClassWithFloatField[] dstData) throws Throwable {
+      for (int i=0; i<dstData.length; ++i) {
+         dstData[i] = srcData[i];
+      }
+
+      for (int i=0; i<dstData.length; ++i) {
+         assertEquals(dstData[i].f, (float)(i+1));
+      }
+   }
+
+   static void writeArrayElementWithLongField(SomeClassWithLongField[] srcData, SomeClassWithLongField[] dstData) throws Throwable {
+      for (int i=0; i<dstData.length; ++i) {
+         dstData[i] = srcData[i];
+      }
+
+      for (int i=0; i<dstData.length; ++i) {
+         assertEquals(dstData[i].l, (long)(i+1));
+      }
+   }
+
+   @Test(priority=1,invocationCount=2)
+	static public void testValueTypeAaload() throws Throwable {
+      int ARRAY_LENGTH = 10;
+      SomeClassWithDoubleField[] data1 = new SomeClassWithDoubleField[ARRAY_LENGTH];
+      SomeClassWithFloatField[]  data2 = new SomeClassWithFloatField[ARRAY_LENGTH];
+      SomeClassWithLongField[]   data3 = new SomeClassWithLongField[ARRAY_LENGTH];
+
+      for (int i=0; i<ARRAY_LENGTH; ++i) {
+         data1[i] = new SomeClassWithDoubleField((double)i);
+         data2[i] = new SomeClassWithFloatField((float)i);
+         data3[i] = new SomeClassWithLongField((long)i);
+      }
+
+      readArrayElementWithDoubleField(data1);
+      readArrayElementWithFloatField(data2);
+      readArrayElementWithLongField(data3);
+   }
+
+   @Test(priority=1,invocationCount=2)
+	static public void testValueTypeAastore() throws Throwable {
+      int ARRAY_LENGTH = 10;
+      SomeClassWithDoubleField[] srcData1 = new SomeClassWithDoubleField[ARRAY_LENGTH];
+      SomeClassWithDoubleField[] dstData1 = new SomeClassWithDoubleField[ARRAY_LENGTH];
+      SomeClassWithFloatField[]  srcData2 = new SomeClassWithFloatField[ARRAY_LENGTH];
+      SomeClassWithFloatField[]  dstData2 = new SomeClassWithFloatField[ARRAY_LENGTH];
+      SomeClassWithLongField[]   srcData3 = new SomeClassWithLongField[ARRAY_LENGTH];
+      SomeClassWithLongField[]   dstData3 = new SomeClassWithLongField[ARRAY_LENGTH];
+
+      for (int i=0; i<ARRAY_LENGTH; ++i) {
+         srcData1[i] = new SomeClassWithDoubleField((double)(i+1));
+         srcData2[i] = new SomeClassWithFloatField((float)(i+1));
+         srcData3[i] = new SomeClassWithLongField((long)(i+1));
+
+         dstData1[i] = new SomeClassWithDoubleField((double)i);
+         dstData2[i] = new SomeClassWithFloatField((float)i);
+         dstData3[i] = new SomeClassWithLongField((long)i);
+      }
+
+      writeArrayElementWithDoubleField(srcData1, dstData1);
+      writeArrayElementWithFloatField(srcData2, dstData2);
+      writeArrayElementWithLongField(srcData3, dstData3);
+   }
 }