From 1f90f7b35516aa818c45d06ec9a6b9589dcda198 Mon Sep 17 00:00:00 2001 From: Henry Zongaro Date: Thu, 24 Jun 2021 17:14:24 -0400 Subject: [PATCH 1/5] Define symbol reference for value type array null store check Define a new method to find or create a symbol reference for a non- helper method that will be used to check whether an array is a value-types array, and if so, whether a null value is being stored to one of its elements. If so, a NullPointerException will be thrown. Signed-off-by: Henry Zongaro --- runtime/compiler/compile/J9SymbolReferenceTable.cpp | 12 ++++++++++++ runtime/compiler/compile/J9SymbolReferenceTable.hpp | 10 ++++++++++ 2 files changed, 22 insertions(+) diff --git a/runtime/compiler/compile/J9SymbolReferenceTable.cpp b/runtime/compiler/compile/J9SymbolReferenceTable.cpp index 130cb41d1af..6d5ea8a7cf5 100644 --- a/runtime/compiler/compile/J9SymbolReferenceTable.cpp +++ b/runtime/compiler/compile/J9SymbolReferenceTable.cpp @@ -2427,6 +2427,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 index 807d94eca63..92eaa2c12d1 100644 --- a/runtime/compiler/compile/J9SymbolReferenceTable.hpp +++ b/runtime/compiler/compile/J9SymbolReferenceTable.hpp @@ -311,6 +311,16 @@ class SymbolReferenceTable : public OMR::SymbolReferenceTableConnector */ TR::SymbolReference *findOrCreateObjectEqualityComparisonSymbolRef(); + /** + * \brief + * Finds the "nonhelper" symbol + * reference, creating it if necessary. + * + * \return + * The symbol reference. + */ + TR::SymbolReference *findOrCreateNonNullableArrayNullStoreCheckSymbolRef(); + /** * \brief * Creates a new symbol for a parameter within the supplied owning method of the From 2b2e73cb489ceec8491a526b0393e2b449351e70 Mon Sep 17 00:00:00 2001 From: Henry Zongaro Date: Tue, 8 Jun 2021 11:47:07 -0400 Subject: [PATCH 2/5] Check for store of non-value type to array element If a value that is to be stored to an array element via a call to the jitStoreFlattenableArrayElement helper is not a value type, value propagation can transform it to use inline code. If the array has a value type as its component type, an ArrayStoreCheckException or a NullPointerException will result, so there is no need to use the helper to handle the possibility of a flattenable array. The test is represented in the IL with a call to the nonNullableArrayStoreNullCheck non-helper, which is expanded in the Tree Lowering optimization into an actual test of the whether the component type of the array is a value type guarding a NULLCHK of the value. This renders obsolete prototype code that overloaded the ArrayStoreCHK opcode to perform this additional checking, so logic Tree Lowering for ArrayStoreCHK is also removed. Signed-off-by: Henry Zongaro --- .../compiler/optimizer/J9ValuePropagation.cpp | 42 ++++- .../compiler/optimizer/J9ValuePropagation.hpp | 2 +- runtime/compiler/optimizer/TreeLowering.cpp | 149 ++++++++---------- 3 files changed, 103 insertions(+), 90 deletions(-) diff --git a/runtime/compiler/optimizer/J9ValuePropagation.cpp b/runtime/compiler/optimizer/J9ValuePropagation.cpp index f5194b6f74f..aee699c5ae8 100644 --- a/runtime/compiler/optimizer/J9ValuePropagation.cpp +++ b/runtime/compiler/optimizer/J9ValuePropagation.cpp @@ -761,19 +761,26 @@ 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; TR::Node *indexNode = node->getChild(elementIndexOpIndex); TR::Node *arrayRefNode = node->getChild(arrayRefOpIndex); + TR::Node *storeValueNode = isStoreFlattenableArrayElement ? node->getChild(storeValueOpIndex) : NULL; 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_YesNoMaybe isStoreValueVT = isStoreFlattenableArrayElement ? isValue(getConstraint(storeValueNode, storeValueGlobal)) : TR_maybe; + + // 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); @@ -781,7 +788,20 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node) 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 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) && !storeValueNode->isNonNull()) + { + flagsForTransform.set(ValueTypesHelperCallTransform::RequiresStoreAndNullCheck); + } + else + { + flagsForTransform.set(ValueTypesHelperCallTransform::RequiresStoreCheck); + } } if (!owningMethodDoesNotContainBoundChecks(this, node)) @@ -1666,6 +1686,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 @@ -1742,12 +1763,21 @@ 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))); + } } else { diff --git a/runtime/compiler/optimizer/J9ValuePropagation.hpp b/runtime/compiler/optimizer/J9ValuePropagation.hpp index efa38749998..e536d302c97 100644 --- 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, }; }; diff --git a/runtime/compiler/optimizer/TreeLowering.cpp b/runtime/compiler/optimizer/TreeLowering.cpp index 0a93c16aa76..d2737093034 100644 --- a/runtime/compiler/optimizer/TreeLowering.cpp +++ b/runtime/compiler/optimizer/TreeLowering.cpp @@ -65,6 +65,7 @@ TR::TreeLowering::perform() { comp()->dumpMethodTrees("Trees after Tree Lowering Optimization"); } + return 0; } @@ -600,10 +601,10 @@ AcmpTransformer::lower(TR::Node * const node, TR::TreeTop * const tt) } -class ArrayStoreCHKTransformer: public TR::TreeLowering::Transformer +class NonNullableArrayNullStoreCheckTransformer: public TR::TreeLowering::Transformer { public: - explicit ArrayStoreCHKTransformer(TR::TreeLowering* opt) + explicit NonNullableArrayNullStoreCheckTransformer(TR::TreeLowering* opt) : TR::TreeLowering::Transformer(opt) {} @@ -620,96 +621,74 @@ class ArrayStoreCHKTransformer: public TR::TreeLowering::Transformer * @param tt is the treetop at the root of the tree ancoring the current node */ void -ArrayStoreCHKTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) +NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) { - // Pattern match the ArrayStoreCHK operands to get the source of the assignment - // (sourceChild) and the array to which an element will have a value assigned (destChild) - TR::Node *firstChild = node->getFirstChild(); - - // The only kind of child for an ArrayStoreCHK should be an awrtbari. If something - // else can appear (such as astorei) will need to rework the logic to determine the - // destination of the element store + // Transform + // +-----------------------------------------+ + // | ttprev | + // | treetop | + // | call | + // | | + // | | + // | ttnext | + // +-----------------------------------------+ // - TR_ASSERT_FATAL_WITH_NODE(node, firstChild->getOpCodeValue() == TR::awrtbari, "Expected child of ArrayStoreCHK to be awrtbari"); - - TR::Node *sourceChild = firstChild->getSecondChild(); - TR::Node *destChild = firstChild->getChild(2); + // into + // +--------------------------------+ + // | ttprev | + // | treetop | + // | | + // | treetop | + // | | + // | ificmpeq -->------------------*---------+ + // | iand | | + // | iloadi | | + // | aloadi | | + // | aloadi | | + // | ==> | | + // | iconst J9ClassIsValueType | | + // | iconst 0 | | + // | BBEnd | | + // +--------------------------------+ | + // | BBStart (Extension) | | + // | NULLCHK | | + // | Passthrough | | + // | ==> | | + // | BBEnd | | + // +--------------------------------+ | + // | | + // +--------------------------+ + // | + // v + // +--------------------------------+ + // | BBStart | + // | ttnext | + // +--------------------------------+ + // + TR::Node *sourceChild = node->getFirstChild(); + TR::Node *destChild = node->getSecondChild(); - // Only need to lower if it is possible that the value is a null reference if (!sourceChild->isNonNull()) { TR::CFG * cfg = comp()->getFlowGraph(); cfg->invalidateStructure(); TR::Block *prevBlock = tt->getEnclosingBlock(); - - performTransformation(comp(), "%sTransforming ArrayStoreCHK n%dn [%p] by splitting block block_%d, and inserting a NULLCHK guarded with a check of whether the component type of the array is a value type\n", optDetailString(), node->getGlobalIndex(), node, prevBlock->getNumber()); - - // Anchor the node containing the source of the array element - // assignment and the node that contains the destination array - // to ensure they are available for the ificmpeq and NULLCHK TR::TreeTop *anchoredArrayTT = TR::TreeTop::create(comp(), tt->getPrevTreeTop(), TR::Node::create(TR::treetop, 1, destChild)); TR::TreeTop *anchoredSourceTT = TR::TreeTop::create(comp(), anchoredArrayTT, TR::Node::create(TR::treetop, 1, sourceChild)); - // Transform - // +--------------------------------+ - // | ttprev | - // | ArrayStoreCHK | - // | awrtbari | - // | aladd | - // | | - // | index-offset-calculation | - // | | - // | | - // +--------------------------------+ - // - // into - // +--------------------------------+ - // | treetop | - // | | - // | treetop | - // | | - // | ificmpeq -->------------------*---------+ - // | iand | | - // | iloadi | | - // | aloadi | | - // | aloadi | | - // | | | - // | iconst J9ClassIsValueType | | - // | iconst 0 | | - // | BBEnd | | - // +--------------------------------+ | - // | BBStart (Extension) | | - // | NULLCHK | | - // | Passthrough | | - // | | | - // | BBEnd | | - // +--------------------------------+ | - // | | - // +--------------------------+ - // | - // v - // +--------------------------------+ - // | BBStart | - // | ArrayStoreCHK | - // | awrtbari | - // | aladd | - // | aload | - // | index-offset-calculation | - // | aload | - // +--------------------------------+ - // - TR::Node *ifNode = comp()->fej9()->checkArrayCompClassValueType(anchoredArrayTT->getNode()->getFirstChild(), TR::ificmpeq); + TR::Node *ifNode = comp()->fej9()->checkArrayCompClassValueType(destChild, TR::ificmpeq); TR::Node *passThru = TR::Node::create(node, TR::PassThrough, 1, sourceChild); - TR::ResolvedMethodSymbol *currentMethod = comp()->getMethodSymbol(); - TR::Block *arrayStoreCheckBlock = prevBlock->splitPostGRA(tt, cfg); + TR::TreeTop *nextTT = tt->getNextTreeTop(); + tt->getPrevTreeTop()->join(nextTT); + TR::Block *nextBlock = prevBlock->splitPostGRA(nextTT, cfg); - ifNode->setBranchDestination(arrayStoreCheckBlock->getEntry()); + ifNode->setBranchDestination(nextBlock->getEntry()); - // Copy register dependencies from the end of the block split before the - // ArrayStoreCHK to the ificmpeq that's being added to the end of that block + // Copy register dependencies from the end of the block split + // to the ificmpeq that's being added to the end of that block if (prevBlock->getExit()->getNode()->getNumChildren() != 0) { TR::Node *blkDeps = prevBlock->getExit()->getNode()->getFirstChild(); @@ -735,6 +714,8 @@ ArrayStoreCHKTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) prevBlock->append(TR::TreeTop::create(comp(), ifNode)); + TR::ResolvedMethodSymbol *currentMethod = comp()->getMethodSymbol(); + TR::Node *nullCheck = TR::Node::createWithSymRef(node, TR::NULLCHK, 1, passThru, comp()->getSymRefTab()->findOrCreateNullCheckSymbolRef(currentMethod)); TR::TreeTop *nullCheckTT = prevBlock->append(TR::TreeTop::create(comp(), nullCheck)); @@ -742,9 +723,13 @@ ArrayStoreCHKTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) TR::Block *nullCheckBlock = prevBlock->split(nullCheckTT, cfg); nullCheckBlock->setIsExtensionOfPreviousBlock(true); - - cfg->addEdge(prevBlock, arrayStoreCheckBlock); } + else + { + tt->getPrevTreeTop()->join(tt->getNextTreeTop()); + } + + node->recursivelyDecReferenceCount(); } static TR::Node * @@ -1536,6 +1521,10 @@ TR::TreeLowering::lowerValueTypeOperations(TransformationManager& transformation transformations.addTransformation(getTransformer(), node, tt); } } + else if (symRefTab->isNonHelper(node->getSymbolReference(), TR::SymbolReferenceTable::nonNullableArrayNullStoreCheckSymbol)) + { + transformations.addTransformation(getTransformer(), node, tt); + } else if (node->getSymbolReference()->getReferenceNumber() == TR_ldFlattenableArrayElement) { static char *disableInliningCheckAaload = feGetEnv("TR_DisableVT_AALOAD_Inlining"); @@ -1560,10 +1549,4 @@ TR::TreeLowering::lowerValueTypeOperations(TransformationManager& transformation } } } - // If inlining check for aastore is enabled, the NULLCHK on the value reference is - // taken care of by StoreArrayElementTransformer. - else if (node->getOpCodeValue() == TR::ArrayStoreCHK && disableInliningCheckAastore) - { - transformations.addTransformation(getTransformer(), node, tt); - } } From dfe4725c7729480df45a03d7b1ecc56b29ae9bcd Mon Sep 17 00:00:00 2001 From: Annabelle Huo Date: Thu, 8 Jul 2021 17:01:58 -0400 Subject: [PATCH 3/5] Add NULL check before checking if array component type is VT When generating inline IL for nonNullableArrayNullStoreCheck in the Tree Lowering optimization, rather than checking whether the component type of an array is a value type and then checking whether the value being stored is null, it should typically be faster to check first whether the value is null. That avoids the double indirection to access the class flags of the component type if the value that's being assigned is non- null. Also added missing edges to CFG for branches around NULLCHK. Authored-by: Annabelle Huo Co-authored-by: Henry Zongaro Signed-off-by: Henry Zongaro --- runtime/compiler/optimizer/TreeLowering.cpp | 132 +++++++++++--------- 1 file changed, 75 insertions(+), 57 deletions(-) diff --git a/runtime/compiler/optimizer/TreeLowering.cpp b/runtime/compiler/optimizer/TreeLowering.cpp index d2737093034..1f87049e9be 100644 --- a/runtime/compiler/optimizer/TreeLowering.cpp +++ b/runtime/compiler/optimizer/TreeLowering.cpp @@ -600,6 +600,32 @@ AcmpTransformer::lower(TR::Node * const node, TR::TreeTop * const tt) } } +static void +copyRegisterDependency(TR::Node *fromNode, TR::Node *toNode) + { + if (fromNode->getNumChildren() != 0) + { + TR::Node *blkDeps = fromNode->getFirstChild(); + TR::Node *newDeps = TR::Node::create(blkDeps, TR::GlRegDeps); + + for (int i = 0; i < blkDeps->getNumChildren(); i++) + { + TR::Node *regDep = blkDeps->getChild(i); + + if (regDep->getOpCodeValue() == TR::PassThrough) + { + TR::Node *orig= regDep; + regDep = TR::Node::create(orig, TR::PassThrough, 1, orig->getFirstChild()); + regDep->setLowGlobalRegisterNumber(orig->getLowGlobalRegisterNumber()); + regDep->setHighGlobalRegisterNumber(orig->getHighGlobalRegisterNumber()); + } + + newDeps->addChildren(®Dep, 1); + } + + toNode->addChildren(&newDeps, 1); + } + } class NonNullableArrayNullStoreCheckTransformer: public TR::TreeLowering::Transformer { @@ -612,10 +638,14 @@ class NonNullableArrayNullStoreCheckTransformer: public TR::TreeLowering::Transf }; /** - * If value types are enabled, and the value that is being assigned to the array - * element might be a null reference, lower the ArrayStoreCHK by splitting the - * block before the ArrayStoreCHK, and inserting a NULLCHK guarded by a check - * of whether the array's component type is a value type. + * If value types are enabled, a check of whether a null reference is being + * assigned to a value type array might be needed. This is represented in the + * IL with a call to the non-helper. + * + * Lower the call by splitting the block before the call, and replacing it + a with a NULLCHK guarded by tests of whether the value is null and whether + * the array's component type is a value type. If the value is known to be + * non-null at this point in the compilation, the call is simply removed. * * @param node is the current node in the tree walk * @param tt is the treetop at the root of the tree ancoring the current node @@ -628,8 +658,8 @@ NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeT // | ttprev | // | treetop | // | call | - // | | // | | + // | | // | ttnext | // +-----------------------------------------+ // @@ -640,6 +670,12 @@ NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeT // | | // | treetop | // | | + // | ifacmpne -->------------------*---------+ + // | ==> | | + // | aconst 0 | | + // | BBEnd | | + // +--------------------------------+ | + // | BBStart (Extension) | | // | ificmpeq -->------------------*---------+ // | iand | | // | iloadi | | @@ -677,52 +713,61 @@ NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeT TR::TreeTop *anchoredArrayTT = TR::TreeTop::create(comp(), tt->getPrevTreeTop(), TR::Node::create(TR::treetop, 1, destChild)); TR::TreeTop *anchoredSourceTT = TR::TreeTop::create(comp(), anchoredArrayTT, TR::Node::create(TR::treetop, 1, sourceChild)); - TR::Node *ifNode = comp()->fej9()->checkArrayCompClassValueType(destChild, TR::ificmpeq); - - TR::Node *passThru = TR::Node::create(node, TR::PassThrough, 1, sourceChild); - TR::TreeTop *nextTT = tt->getNextTreeTop(); tt->getPrevTreeTop()->join(nextTT); TR::Block *nextBlock = prevBlock->splitPostGRA(nextTT, cfg); + TR::Node *ifNode = comp()->fej9()->checkArrayCompClassValueType(destChild, TR::ificmpeq); + ifNode->setBranchDestination(nextBlock->getEntry()); // Copy register dependencies from the end of the block split - // to the ificmpeq that's being added to the end of that block - if (prevBlock->getExit()->getNode()->getNumChildren() != 0) - { - TR::Node *blkDeps = prevBlock->getExit()->getNode()->getFirstChild(); - TR::Node *ifDeps = TR::Node::create(blkDeps, TR::GlRegDeps); + // to the ificmpeq, which checks for a value type, that's being + // added to the end of that block + // + copyRegisterDependency(prevBlock->getExit()->getNode(), ifNode); - for (int i = 0; i < blkDeps->getNumChildren(); i++) - { - TR::Node *regDep = blkDeps->getChild(i); + TR::TreeTop *ifArrayCompClassValueTypeTT = prevBlock->append(TR::TreeTop::create(comp(), ifNode)); - if (regDep->getOpCodeValue() == TR::PassThrough) - { - TR::Node *orig= regDep; - regDep = TR::Node::create(orig, TR::PassThrough, 1, orig->getFirstChild()); - regDep->setLowGlobalRegisterNumber(orig->getLowGlobalRegisterNumber()); - regDep->setHighGlobalRegisterNumber(orig->getHighGlobalRegisterNumber()); - } + bool enableTrace = trace(); + auto* const nullConst = TR::Node::aconst(0); + auto* const checkValueNull = TR::Node::createif(TR::ifacmpne, sourceChild, nullConst, nextBlock->getEntry()); - ifDeps->addChildren(®Dep, 1); - } + // Copy register dependencies from the end of the block split to the + // ifacmpne, which checks for a null value, that's being added to the + // end of that block + // + copyRegisterDependency(prevBlock->getExit()->getNode(), checkValueNull); - ifNode->addChildren(&ifDeps, 1); + TR::TreeTop *checkValueNullTT = ifArrayCompClassValueTypeTT->insertBefore(TR::TreeTop::create(comp(), checkValueNull)); + + if (enableTrace) + { + traceMsg(comp(),"checkValueNull n%dn is inserted before n%dn in prevBlock %d\n", checkValueNull->getGlobalIndex(), ifNode->getGlobalIndex(), prevBlock->getNumber()); } - prevBlock->append(TR::TreeTop::create(comp(), ifNode)); + TR::Block *compTypeTestBlock = prevBlock->split(ifArrayCompClassValueTypeTT, cfg); + compTypeTestBlock->setIsExtensionOfPreviousBlock(true); + + cfg->addEdge(prevBlock, nextBlock); + + if (enableTrace) + { + traceMsg(comp(),"ifArrayCompClassValueTypeTT n%dn is isolated in compTypeTestBlock %d\n", ifNode->getGlobalIndex(), compTypeTestBlock->getNumber()); + } TR::ResolvedMethodSymbol *currentMethod = comp()->getMethodSymbol(); + TR::Node *passThru = TR::Node::create(node, TR::PassThrough, 1, sourceChild); TR::Node *nullCheck = TR::Node::createWithSymRef(node, TR::NULLCHK, 1, passThru, comp()->getSymRefTab()->findOrCreateNullCheckSymbolRef(currentMethod)); - TR::TreeTop *nullCheckTT = prevBlock->append(TR::TreeTop::create(comp(), nullCheck)); + TR::TreeTop *nullCheckTT = compTypeTestBlock->append(TR::TreeTop::create(comp(), nullCheck)); - TR::Block *nullCheckBlock = prevBlock->split(nullCheckTT, cfg); + TR::Block *nullCheckBlock = compTypeTestBlock->split(nullCheckTT, cfg); nullCheckBlock->setIsExtensionOfPreviousBlock(true); + + cfg->addEdge(compTypeTestBlock, nextBlock); } else { @@ -806,33 +851,6 @@ createStoreNodeForAnchoredNode(TR::Node *anchoredNode, TR::Node *nodeToBeStored, return storeNode; } -static void -copyRegisterDependency(TR::Node *fromNode, TR::Node *toNode) - { - if (fromNode->getNumChildren() != 0) - { - TR::Node *blkDeps = fromNode->getFirstChild(); - TR::Node *newDeps = TR::Node::create(blkDeps, TR::GlRegDeps); - - for (int i = 0; i < blkDeps->getNumChildren(); i++) - { - TR::Node *regDep = blkDeps->getChild(i); - - if (regDep->getOpCodeValue() == TR::PassThrough) - { - TR::Node *orig= regDep; - regDep = TR::Node::create(orig, TR::PassThrough, 1, orig->getFirstChild()); - regDep->setLowGlobalRegisterNumber(orig->getLowGlobalRegisterNumber()); - regDep->setHighGlobalRegisterNumber(orig->getHighGlobalRegisterNumber()); - } - - newDeps->addChildren(®Dep, 1); - } - - toNode->addChildren(&newDeps, 1); - } - } - class LoadArrayElementTransformer: public TR::TreeLowering::Transformer { public: From 923e871d65f10b6e53ad7799f7f384a575c1b914 Mon Sep 17 00:00:00 2001 From: Henry Zongaro Date: Mon, 9 Aug 2021 14:42:45 -0400 Subject: [PATCH 4/5] Invalidate aliasing if BNDCHK or ArrayStoreCHK are generated If Value Propagation or Tree Lowering generate BNDCHK or ArrayStoreCHK operations, the alias sets for the checking symbol reference might not have been constructed. Mark alias sets as invalid so they can be rebuilt if they are needed after those optimizations. Signed-off-by: Henry Zongaro --- runtime/compiler/optimizer/J9ValuePropagation.cpp | 10 ++++++++++ runtime/compiler/optimizer/TreeLowering.cpp | 15 +++++++++++++++ runtime/compiler/optimizer/TreeLowering.hpp | 2 ++ 3 files changed, 27 insertions(+) diff --git a/runtime/compiler/optimizer/J9ValuePropagation.cpp b/runtime/compiler/optimizer/J9ValuePropagation.cpp index aee699c5ae8..94b3fae450f 100644 --- a/runtime/compiler/optimizer/J9ValuePropagation.cpp +++ b/runtime/compiler/optimizer/J9ValuePropagation.cpp @@ -1738,6 +1738,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); @@ -1778,6 +1783,11 @@ J9::ValuePropagation::doDelayedTransformations() 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/TreeLowering.cpp b/runtime/compiler/optimizer/TreeLowering.cpp index 1f87049e9be..8a61d2cf4f0 100644 --- a/runtime/compiler/optimizer/TreeLowering.cpp +++ b/runtime/compiler/optimizer/TreeLowering.cpp @@ -1095,6 +1095,11 @@ LoadArrayElementTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) arraylengthNode->setArrayStride(dataWidth); elementLoadTT->insertBefore(TR::TreeTop::create(comp, TR::Node::createWithSymRef(TR::BNDCHK, 2, 2, arraylengthNode, anchoredElementIndexNode, comp->getSymRefTab()->findOrCreateArrayBoundsCheckSymbolRef(comp->getMethodSymbol())))); + + // This might be the first time the various checking symbol references are used + // Need to ensure aliasing for them is correctly constructed + // + this->optimizer()->setAliasSetsAreValid(false); } @@ -1401,6 +1406,11 @@ StoreArrayElementTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) arrayStoreTT = originalBlock->append(TR::TreeTop::create(comp, arrayStoreCHKNode)); if (enableTrace) traceMsg(comp, "Created arrayStoreCHK treetop n%dn arrayStoreCHKNode n%dn\n", arrayStoreTT->getNode()->getGlobalIndex(), arrayStoreCHKNode->getGlobalIndex()); + + // This might be the first time the various checking symbol references are used + // Need to ensure aliasing for them is correctly constructed + // + this->optimizer()->setAliasSetsAreValid(false); } else { @@ -1446,6 +1456,11 @@ StoreArrayElementTransformer::lower(TR::Node* const node, TR::TreeTop* const tt) { arrayStoreTT->insertBefore(TR::TreeTop::create(comp, TR::Node::createWithSymRef(TR::BNDCHK, 2, 2, arraylengthNode, anchoredElementIndexNode, comp->getSymRefTab()->findOrCreateArrayBoundsCheckSymbolRef(comp->getMethodSymbol())))); } + + // This might be the first time the various checking symbol references are used + // Need to ensure aliasing for them is correctly constructed + // + this->optimizer()->setAliasSetsAreValid(false); } if (comp->useCompressedPointers()) diff --git a/runtime/compiler/optimizer/TreeLowering.hpp b/runtime/compiler/optimizer/TreeLowering.hpp index 3df37866fe0..aff9fe55f47 100644 --- a/runtime/compiler/optimizer/TreeLowering.hpp +++ b/runtime/compiler/optimizer/TreeLowering.hpp @@ -67,6 +67,8 @@ class TreeLowering : public TR::Optimization TR::Compilation* comp() { return _comp; } + TR::Optimizer* optimizer() { return _treeLoweringOpt->optimizer(); } + bool trace() { return _treeLoweringOpt->trace(); } const char* optDetailString() { return _treeLoweringOpt->optDetailString(); } From 7f95a3bc722fdb4103b51cc8cc55db5ca900430a Mon Sep 17 00:00:00 2001 From: Henry Zongaro Date: Thu, 9 Sep 2021 13:44:32 -0400 Subject: [PATCH 5/5] Check non-null constraint rather than isNonNull on node in VP When Value Propagation determines that a call to can be transformed due to constraints associated with the value that is being stored, it should determine whether the value is known to be non-null by checking the constraint associated with the value rather than calling isNonNull() on the node. Signed-off-by: Henry Zongaro --- .../compiler/optimizer/J9ValuePropagation.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/runtime/compiler/optimizer/J9ValuePropagation.cpp b/runtime/compiler/optimizer/J9ValuePropagation.cpp index 94b3fae450f..820d70803bc 100644 --- a/runtime/compiler/optimizer/J9ValuePropagation.cpp +++ b/runtime/compiler/optimizer/J9ValuePropagation.cpp @@ -768,11 +768,19 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node) TR::Node *indexNode = node->getChild(elementIndexOpIndex); TR::Node *arrayRefNode = node->getChild(arrayRefOpIndex); - TR::Node *storeValueNode = isStoreFlattenableArrayElement ? node->getChild(storeValueOpIndex) : NULL; TR::VPConstraint *arrayConstraint = getConstraint(arrayRefNode, arrayRefGlobal); TR_YesNoMaybe isCompTypeVT = isArrayCompTypeValueType(arrayConstraint); - TR_YesNoMaybe isStoreValueVT = isStoreFlattenableArrayElement ? isValue(getConstraint(storeValueNode, storeValueGlobal)) : TR_maybe; + 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 @@ -789,12 +797,12 @@ J9::ValuePropagation::constrainRecognizedMethod(TR::Node *node) if (isStoreFlattenableArrayElement && !owningMethodDoesNotContainStoreChecks(this, node)) { // 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 null, both a run-time + // 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) && !storeValueNode->isNonNull()) + if ((isCompTypeVT != TR_no) && (storeValueConstraint == NULL || !storeValueConstraint->isNonNullObject())) { flagsForTransform.set(ValueTypesHelperCallTransform::RequiresStoreAndNullCheck); }