Merge pull request #13403 from hzongaro/check-aastore-value-is-VT-PR

Allow Value Propagation to transform an array store if the value assigned is known not to be a value type
eclipse-openj9 · Nov 11, 2021 · c4bbd37 · c4bbd37
2 parents 2022b38 + 7f95a3b
commit c4bbd37
Show file tree

Hide file tree

Showing 6 changed files with 228 additions and 140 deletions.
diff --git a/runtime/compiler/compile/J9SymbolReferenceTable.cpp b/runtime/compiler/compile/J9SymbolReferenceTable.cpp
@@ -2477,6 +2477,18 @@ J9::SymbolReferenceTable::findOrCreateObjectEqualityComparisonSymbolRef()
    return symRef;
    }
 
+TR::SymbolReference *
+J9::SymbolReferenceTable::findOrCreateNonNullableArrayNullStoreCheckSymbolRef()
+   {
+   TR::SymbolReference *symRef = element(nonNullableArrayNullStoreCheckSymbol);
+   if (symRef != NULL)
+      return symRef;
+
+   symRef = self()->findOrCreateCodeGenInlinedHelper(nonNullableArrayNullStoreCheckSymbol);
+   symRef->setCanGCandExcept();
+   return symRef;
+   }
+
 TR::ParameterSymbol *
 J9::SymbolReferenceTable::createParameterSymbol(
       TR::ResolvedMethodSymbol *owningMethodSymbol,

diff --git a/runtime/compiler/compile/J9SymbolReferenceTable.hpp b/runtime/compiler/compile/J9SymbolReferenceTable.hpp
@@ -340,6 +340,16 @@ class SymbolReferenceTable : public OMR::SymbolReferenceTableConnector
    TR::SymbolReference *findOrCreateObjectEqualityComparisonSymbolRef();
    TR::SymbolReference *findOrCreateObjectInequalityComparisonSymbolRef();
 
+   /**
+    * \brief
+    *    Finds the <nonNullableArrayNullStoreCheck> "nonhelper" symbol
+    *    reference, creating it if necessary.
+    *
+    *  \return
+    *     The <nonNullableArrayNullStoreCheck> symbol reference.
+    */
+   TR::SymbolReference *findOrCreateNonNullableArrayNullStoreCheckSymbolRef();
+
    /**
     * \brief
     *    Creates a new symbol for a parameter within the supplied owning method of the

diff --git a/runtime/compiler/optimizer/J9ValuePropagation.cpp b/runtime/compiler/optimizer/J9ValuePropagation.cpp
@@ -775,6 +775,8 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node)
          }
 
       bool arrayRefGlobal;
+      bool storeValueGlobal;
+      const int storeValueOpIndex = isLoadFlattenableArrayElement ? -1 : 0;
       const int elementIndexOpIndex = isLoadFlattenableArrayElement ? 0 : 1;
       const int arrayRefOpIndex = elementIndexOpIndex+1;
 
@@ -783,19 +785,45 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node)
       TR::VPConstraint *arrayConstraint = getConstraint(arrayRefNode, arrayRefGlobal);
       TR_YesNoMaybe isCompTypeVT = isArrayCompTypeValueType(arrayConstraint);
 
-      // If the array's component type is definitely not a value type, add a delayed
-      // transformation to replace the helper call with inline code to perform the
-      // array element access
+      TR::Node *storeValueNode = NULL;
+      TR::VPConstraint *storeValueConstraint = NULL;
+      TR_YesNoMaybe isStoreValueVT = TR_maybe;
+
+      if (isStoreFlattenableArrayElement)
+         {
+         storeValueNode = node->getChild(storeValueOpIndex);
+         storeValueConstraint = getConstraint(storeValueNode, storeValueGlobal);
+         isStoreValueVT = isValue(storeValueConstraint);
+         }
+
+      // If the array's component type is definitely not a value type, or if the value
+      // being assigned in an array store operation is definitely not a value type, add
+      // a delayed transformation to replace the helper call with inline code to
+      // perform the array element access.
       //
-      if (arrayConstraint != NULL && isCompTypeVT == TR_no)
+      if ((arrayConstraint != NULL && isCompTypeVT == TR_no)
+          || (isStoreFlattenableArrayElement && isStoreValueVT == TR_no))
          {
          flags8_t flagsForTransform(isLoadFlattenableArrayElement ? ValueTypesHelperCallTransform::IsArrayLoad
                                                                   : ValueTypesHelperCallTransform::IsArrayStore);
          flagsForTransform.set(ValueTypesHelperCallTransform::InsertDebugCounter);
 
          if (isStoreFlattenableArrayElement && !owningMethodDoesNotContainStoreChecks(this, node))
             {
-            flagsForTransform.set(ValueTypesHelperCallTransform::RequiresStoreCheck);
+            // If storing to an array whose component type is or might be a value type
+            // and the value that's being assigned is or might be null, both a run-time
+            // NULLCHK of the value is required (guarded by a check of whether the
+            // component type is a value type) and an ArrayStoreCHK are required;
+            // otherwise, only the ArrayStoreCHK is required.
+            //
+            if ((isCompTypeVT != TR_no) && (storeValueConstraint == NULL || !storeValueConstraint->isNonNullObject()))
+               {
+               flagsForTransform.set(ValueTypesHelperCallTransform::RequiresStoreAndNullCheck);
+               }
+            else
+               {
+               flagsForTransform.set(ValueTypesHelperCallTransform::RequiresStoreCheck);
+               }
             }
 
          if (!owningMethodDoesNotContainBoundChecks(this, node))
@@ -1738,6 +1766,7 @@ J9::ValuePropagation::doDelayedTransformations()
       const bool isStore = callToTransform->_flags.testAny(ValueTypesHelperCallTransform::IsArrayStore);
       const bool isCompare = callToTransform->_flags.testAny(ValueTypesHelperCallTransform::IsRefCompare);
       const bool needsStoreCheck = callToTransform->_flags.testAny(ValueTypesHelperCallTransform::RequiresStoreCheck);
+      const bool needsStoreAndNullCheck = callToTransform->_flags.testAny(ValueTypesHelperCallTransform::RequiresStoreAndNullCheck);
       const bool needsBoundCheck = callToTransform->_flags.testAny(ValueTypesHelperCallTransform::RequiresBoundCheck);
 
       // performTransformation was already checked for comparison non-helper call
@@ -1789,6 +1818,11 @@ J9::ValuePropagation::doDelayedTransformations()
          TR::Node *bndChkNode = TR::Node::createWithSymRef(TR::BNDCHK, 2, 2, arrayLengthNode, indexNode,
                                              comp()->getSymRefTab()->findOrCreateArrayBoundsCheckSymbolRef(comp()->getMethodSymbol()));
          callTree->insertBefore(TR::TreeTop::create(comp(), bndChkNode));
+
+         // This might be the first time the array bounds check symbol reference is used
+         // Need to ensure aliasing for them is correctly constructed
+         //
+         optimizer()->setAliasSetsAreValid(false);
          }
 
       TR::SymbolReference *elementSymRef = comp()->getSymRefTab()->findOrCreateArrayShadowSymbolRef(TR::Address, arrayRefNode);
@@ -1814,12 +1848,26 @@ J9::ValuePropagation::doDelayedTransformations()
          TR::Node *elementStoreNode = TR::Node::recreateWithoutProperties(callNode, TR::awrtbari, 3, elementAddressNode,
                                                    valueNode, arrayRefNode, elementSymRef);
 
-         if (needsStoreCheck)
+         if (needsStoreCheck || needsStoreAndNullCheck)
             {
             TR::ResolvedMethodSymbol *methodSym = comp()->getMethodSymbol();
             TR::SymbolReference *storeCheckSymRef = comp()->getSymRefTab()->findOrCreateTypeCheckArrayStoreSymbolRef(methodSym);
             TR::Node *storeCheckNode = TR::Node::createWithRoomForThree(TR::ArrayStoreCHK, elementStoreNode, 0, storeCheckSymRef);
+            storeCheckNode->setByteCodeInfo(elementStoreNode->getByteCodeInfo());
             callTree->setNode(storeCheckNode);
+
+            if (needsStoreAndNullCheck)
+               {
+               TR::SymbolReference *nonNullableArrayNullStoreCheckSymRef = comp()->getSymRefTab()->findOrCreateNonNullableArrayNullStoreCheckSymbolRef();
+               TR::Node *nullCheckNode = TR::Node::createWithSymRef(TR::call, 2, 2, valueNode, arrayRefNode, nonNullableArrayNullStoreCheckSymRef);
+               nullCheckNode->setByteCodeInfo(elementStoreNode->getByteCodeInfo());
+               callTree->insertBefore(TR::TreeTop::create(comp(), TR::Node::create(TR::treetop, 1,  nullCheckNode)));
+               }
+
+            // This might be the first time the various checking symbol references are used
+            // Need to ensure aliasing for them is correctly constructed
+            //
+            optimizer()->setAliasSetsAreValid(false);
             }
          else
             {

diff --git a/runtime/compiler/optimizer/J9ValuePropagation.hpp b/runtime/compiler/optimizer/J9ValuePropagation.hpp
@@ -151,7 +151,7 @@ class ValuePropagation : public OMR::ValuePropagation
          IsRefCompare       = 0x08,
          InsertDebugCounter = 0x10,
          RequiresBoundCheck = 0x20,
-         Unused2            = 0x40,
+         RequiresStoreAndNullCheck = 0x40,
          Unused1            = 0x80,
          };
    };