From e5cfdd379281b7e05020b9c5e72c3cffb7811fbd Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Wed, 8 Jan 2020 18:08:19 -0800 Subject: [PATCH] Improvements for null check folding. optFoldNullChecks attempts to remove GT_NULLCHECK nodes that are post-dominated by indirections on the same variable. These changes implement a number of improvements. 1. Recognize more patterns. Before these changes only the following pattern was recognized: x = comma(nullcheck(y), add(y, const1)) followed by indir(add(x, const2)) where const1 + const2 is sufficiently small. With these changes the following patterns are recognized: nullcheck(x) or x = comma(nullcheck(y), add(y, const1)) followed by indir(x) or indir(add(x, const2)) where const1 + const2 is sufficiently small. 2. Indirections now include GT_ARR_LENGTH nodes. 3. Morph has an optimization ((x+icon1)+icon2) => (x+(icon1+icon2)) These changes generalize it to handle commas: ((comma(y, x+icon1)+icon2) => comma(y, x+(icon1+icon2)) That exposes more trees to null check folding. 4. Fix a bug in flow transformations that could lose BBF_HAS_NULLCHECK flag on some basic blocks, which led to missing opportunities for null check folding. 5. Make safety checks in optCanMoveNullCheckPastTree (for trees between the nullcheck and the indirection) both more correct and less conservative. For example, we were not allowing any assignments if we were inside try; however, assignments to compiler temps are safe since they won't be visible in handlers. 5. Increase the maximum number of trees we check between GT_NULLCHECK and the indirection from 25 to 50. 7. Refactor the code and move pattern recognition and safety checks to helper methods. 8. Add missing BBF_HAS_NULLCHECK and OMF_HAS_NULLCHECK when we create GT_NULLCHECK nodes. --- src/coreclr/format.patch | 47 +++ src/coreclr/src/jit/block.cpp | 4 + src/coreclr/src/jit/block.h | 8 +- src/coreclr/src/jit/compiler.h | 16 +- src/coreclr/src/jit/compmemkind.h | 1 + src/coreclr/src/jit/earlyprop.cpp | 511 ++++++++++++++++++++---------- src/coreclr/src/jit/importer.cpp | 10 +- src/coreclr/src/jit/morph.cpp | 45 +-- 8 files changed, 441 insertions(+), 201 deletions(-) create mode 100644 src/coreclr/format.patch diff --git a/src/coreclr/format.patch b/src/coreclr/format.patch new file mode 100644 index 0000000000000..3759a98288680 --- /dev/null +++ b/src/coreclr/format.patch @@ -0,0 +1,47 @@ +diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp +index a71c325ff48..c0569355c89 100644 +--- a/src/coreclr/src/jit/importer.cpp ++++ b/src/coreclr/src/jit/importer.cpp +@@ -15184,41 +15184,41 @@ void Compiler::impImportBlockCode(BasicBlock* block) + info.compCompHnd->compareTypesForEquality(resolvedToken.hClass, clsHnd); + + if (compare == TypeCompareState::Must) + { + JITDUMP("\nOptimizing %s (%s) -- type test will succeed\n", + opcode == CEE_UNBOX ? "UNBOX" : "UNBOX.ANY", eeGetClassName(clsHnd)); + + // For UNBOX, null check (if necessary), and then leave the box payload byref on the stack. + if (opcode == CEE_UNBOX) + { + GenTree* cloneOperand; + op1 = impCloneExpr(op1, &cloneOperand, NO_CLASS_HANDLE, (unsigned)CHECK_SPILL_ALL, + nullptr DEBUGARG("optimized unbox clone")); + + GenTree* boxPayloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* boxPayloadAddress = + gtNewOperNode(GT_ADD, TYP_BYREF, cloneOperand, boxPayloadOffset); + GenTree* nullcheck = gtNewOperNode(GT_NULLCHECK, TYP_I_IMPL, op1); + block->bbFlags |= BBF_HAS_NULLCHECK; + optMethodFlags |= OMF_HAS_NULLCHECK; +- GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress); ++ GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress); + impPushOnStack(result, tiRetVal); + break; + } + + // For UNBOX.ANY load the struct from the box payload byref (the load will nullcheck) + assert(opcode == CEE_UNBOX_ANY); + GenTree* boxPayloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* boxPayloadAddress = gtNewOperNode(GT_ADD, TYP_BYREF, op1, boxPayloadOffset); + impPushOnStack(boxPayloadAddress, tiRetVal); + oper = GT_OBJ; + goto OBJ; + } + else + { + JITDUMP("\nUnable to optimize %s -- can't resolve type comparison\n", + opcode == CEE_UNBOX ? "UNBOX" : "UNBOX.ANY"); + } + } + else + { diff --git a/src/coreclr/src/jit/block.cpp b/src/coreclr/src/jit/block.cpp index 62242d8d89934..c44b26219e15c 100644 --- a/src/coreclr/src/jit/block.cpp +++ b/src/coreclr/src/jit/block.cpp @@ -333,6 +333,10 @@ void BasicBlock::dspFlags() { printf("newobj "); } + if (bbFlags & BBF_HAS_NULLCHECK) + { + printf("nullcheck "); + } #if defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_) if (bbFlags & BBF_FINALLY_TARGET) { diff --git a/src/coreclr/src/jit/block.h b/src/coreclr/src/jit/block.h index c4ef847b11ea4..79e11ed39a81f 100644 --- a/src/coreclr/src/jit/block.h +++ b/src/coreclr/src/jit/block.h @@ -465,7 +465,7 @@ struct BasicBlock : private LIR::Range #define BBF_COMPACT_UPD \ (BBF_CHANGED | BBF_GC_SAFE_POINT | BBF_HAS_JMP | BBF_NEEDS_GCPOLL | BBF_HAS_IDX_LEN | BBF_BACKWARD_JUMP | \ - BBF_HAS_NEWARRAY | BBF_HAS_NEWOBJ) + BBF_HAS_NEWARRAY | BBF_HAS_NEWOBJ | BBF_HAS_NULLCHECK) // Flags a block should not have had before it is split. @@ -481,14 +481,14 @@ struct BasicBlock : private LIR::Range // Flags gained by the bottom block when a block is split. // Note, this is a conservative guess. -// For example, the bottom block might or might not have BBF_HAS_NEWARRAY, -// but we assume it has BBF_HAS_NEWARRAY. +// For example, the bottom block might or might not have BBF_HAS_NEWARRAY or BBF_HAS_NULLCHECK, +// but we assume it has BBF_HAS_NEWARRAY and BBF_HAS_NULLCHECK. // TODO: Should BBF_RUN_RARELY be added to BBF_SPLIT_GAINED ? #define BBF_SPLIT_GAINED \ (BBF_DONT_REMOVE | BBF_HAS_LABEL | BBF_HAS_JMP | BBF_BACKWARD_JUMP | BBF_HAS_IDX_LEN | BBF_HAS_NEWARRAY | \ - BBF_PROF_WEIGHT | BBF_HAS_NEWOBJ | BBF_KEEP_BBJ_ALWAYS | BBF_CLONED_FINALLY_END) + BBF_PROF_WEIGHT | BBF_HAS_NEWOBJ | BBF_KEEP_BBJ_ALWAYS | BBF_CLONED_FINALLY_END | BBF_HAS_NULLCHECK) #ifndef __GNUC__ // GCC doesn't like C_ASSERT at global scope static_assert_no_msg((BBF_SPLIT_NONEXIST & BBF_SPLIT_LOST) == 0); diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 23c692e1ffdd9..938b717827691 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6396,17 +6396,27 @@ class Compiler OPK_NULLCHECK }; + typedef JitHashTable, GenTree*> LocalNumberToNullCheckTreeMap; + bool gtIsVtableRef(GenTree* tree); GenTree* getArrayLengthFromAllocation(GenTree* tree); GenTree* getObjectHandleNodeFromAllocation(GenTree* tree); GenTree* optPropGetValueRec(unsigned lclNum, unsigned ssaNum, optPropKind valueKind, int walkDepth); GenTree* optPropGetValue(unsigned lclNum, unsigned ssaNum, optPropKind valueKind); - GenTree* optEarlyPropRewriteTree(GenTree* tree); + GenTree* optEarlyPropRewriteTree(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap); bool optDoEarlyPropForBlock(BasicBlock* block); bool optDoEarlyPropForFunc(); void optEarlyProp(); - void optFoldNullCheck(GenTree* tree); - bool optCanMoveNullCheckPastTree(GenTree* tree, bool isInsideTry); + void optFoldNullCheck(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap); + GenTree* optFindNullCheckToFold(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap); + bool optIsNullCheckFoldingLegal(GenTree* tree, + GenTree* nullCheckTree, + GenTree** nullCheckParent, + Statement** nullCheckStmt); + bool optCanMoveNullCheckPastTree(GenTree* tree, + unsigned nullCheckLclNum, + bool isInsideTry, + bool checkSideEffectSummary); #if ASSERTION_PROP /************************************************************************** diff --git a/src/coreclr/src/jit/compmemkind.h b/src/coreclr/src/jit/compmemkind.h index 986c4e2dce180..f680b04987d45 100644 --- a/src/coreclr/src/jit/compmemkind.h +++ b/src/coreclr/src/jit/compmemkind.h @@ -57,6 +57,7 @@ CompMemKindMacro(ObjectAllocator) CompMemKindMacro(VariableLiveRanges) CompMemKindMacro(ClassLayout) CompMemKindMacro(TailMergeThrows) +CompMemKindMacro(EarlyProp) //clang-format on #undef CompMemKindMacro diff --git a/src/coreclr/src/jit/earlyprop.cpp b/src/coreclr/src/jit/earlyprop.cpp index 9d06857a1d86b..64f6e98cb890a 100644 --- a/src/coreclr/src/jit/earlyprop.cpp +++ b/src/coreclr/src/jit/earlyprop.cpp @@ -183,6 +183,9 @@ void Compiler::optEarlyProp() compCurBB = block; + CompAllocator allocator(getAllocator(CMK_EarlyProp)); + LocalNumberToNullCheckTreeMap nullCheckMap(allocator); + for (Statement* stmt = block->firstStmt(); stmt != nullptr;) { // Preserve the next link before the propagation and morph. @@ -195,7 +198,7 @@ void Compiler::optEarlyProp() bool isRewritten = false; for (GenTree* tree = stmt->GetTreeList(); tree != nullptr; tree = tree->gtNext) { - GenTree* rewrittenTree = optEarlyPropRewriteTree(tree); + GenTree* rewrittenTree = optEarlyPropRewriteTree(tree, &nullCheckMap); if (rewrittenTree != nullptr) { gtUpdateSideEffects(stmt, rewrittenTree); @@ -229,26 +232,35 @@ void Compiler::optEarlyProp() // // Arguments: // tree - The input tree node to be rewritten. +// nullCheckMap - Map of the local numbers to the latest NULLCHECKs on those locals in the current basic block. // // Return Value: // Return a new tree if the original tree was successfully rewritten. // The containing tree links are updated. // -GenTree* Compiler::optEarlyPropRewriteTree(GenTree* tree) +GenTree* Compiler::optEarlyPropRewriteTree(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap) { GenTree* objectRefPtr = nullptr; optPropKind propKind = optPropKind::OPK_INVALID; + if (tree->OperIsIndirOrArrLength()) + { + // optFoldNullCheck takes care of updating statement info if a null check is removed. + optFoldNullCheck(tree, nullCheckMap); + } + else + { + return nullptr; + } + if (tree->OperGet() == GT_ARR_LENGTH) { objectRefPtr = tree->AsOp()->gtOp1; propKind = optPropKind::OPK_ARRAYLEN; } - else if (tree->OperIsIndir()) + else { - // optFoldNullCheck takes care of updating statement info if a null check is removed. - optFoldNullCheck(tree); - + assert(tree->OperIsIndir()); if (gtIsVtableRef(tree)) { // Don't propagate type handles that are used as null checks, which are usually in @@ -269,10 +281,6 @@ GenTree* Compiler::optEarlyPropRewriteTree(GenTree* tree) return nullptr; } } - else - { - return nullptr; - } if (!objectRefPtr->OperIsScalarLocal() || !lvaInSsa(objectRefPtr->AsLclVarCommon()->GetLclNum())) @@ -477,208 +485,363 @@ GenTree* Compiler::optPropGetValueRec(unsigned lclNum, unsigned ssaNum, optPropK } //---------------------------------------------------------------- -// optFoldNullChecks: Try to find a GT_NULLCHECK node that can be folded into the GT_INDIR node. +// optFoldNullChecks: Try to find a GT_NULLCHECK node that can be folded into the indirection node mark it for removal +// if possible. // // Arguments: -// tree - The input GT_INDIR tree. +// tree - The input indirection tree. +// nullCheckMap - Map of the local numbers to the latest NULLCHECKs on those locals in the current basic block // +// Notes: +// If a GT_NULLCHECK node is post-dominated by an indirection node on the same local and the trees between +// the GT_NULLCHECK and the indirection don't have unsafe side effects, the GT_NULLCHECK can be removed. +// The indir will cause a NullReferenceException if and only if GT_NULLCHECK will cause the same +// NullReferenceException. -void Compiler::optFoldNullCheck(GenTree* tree) +void Compiler::optFoldNullCheck(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap) { - // - // Check for a pattern like this: - // - // = - // / \. - // x comma - // / \. - // nullcheck + - // | / \. - // y y const - // - // - // some trees in the same - // basic block with - // no unsafe side effects - // - // indir - // | - // x - // - // where the const is suitably small - // and transform it into - // - // = - // / \. - // x + - // / \. - // y const - // - // - // some trees with no unsafe side effects here - // - // indir - // | - // x - if ((compCurBB->bbFlags & BBF_HAS_NULLCHECK) == 0) { return; } - assert(tree->OperIsIndir()); + GenTree* nullCheckTree = optFindNullCheckToFold(tree, nullCheckMap); + GenTree* nullCheckParent = nullptr; + Statement* nullCheckStmt = nullptr; + if ((nullCheckTree != nullptr) && optIsNullCheckFoldingLegal(tree, nullCheckTree, &nullCheckParent, &nullCheckStmt)) + { +#ifdef DEBUG + if (verbose) + { + printf("optEarlyProp Marking a null check for removal\n"); + gtDispTree(nullCheckTree); + printf("\n"); + } +#endif + // Remove the null check + nullCheckTree->gtFlags &= ~(GTF_EXCEPT | GTF_DONT_CSE); + + // Set this flag to prevent reordering + nullCheckTree->gtFlags |= GTF_ORDER_SIDEEFF; + nullCheckTree->gtFlags |= GTF_IND_NONFAULTING; + + if (nullCheckParent != nullptr) + { + nullCheckParent->gtFlags &= ~GTF_DONT_CSE; + } + + nullCheckMap->Remove(nullCheckTree->gtGetOp1()->AsLclVarCommon()->GetLclNum()); + + // Re-morph the statement. + Statement* curStmt = compCurStmt; + fgMorphBlockStmt(compCurBB, nullCheckStmt DEBUGARG("optFoldNullCheck")); + compCurStmt = curStmt; + } + + if ((tree->OperGet() == GT_NULLCHECK) && (tree->gtGetOp1()->OperGet() == GT_LCL_VAR)) + { + nullCheckMap->Set(tree->gtGetOp1()->AsLclVarCommon()->GetLclNum(), tree, + LocalNumberToNullCheckTreeMap::SetKind::Overwrite); + } +} + +//---------------------------------------------------------------- +// optFindNullCheckToFold: Try to find a GT_NULLCHECK node that can be folded into the indirection node. +// +// Arguments: +// tree - The input indirection tree. +// nullCheckMap - Map of the local numbers to the latest NULLCHECKs on those locals in the current basic block +// +// Notes: +// Check for cases where +// 1. One of the following trees +// +// nullcheck(x) +// or +// x = comma(nullcheck(y), add(y, const1)) +// +// is post-dominated in the same basic block by one of the following trees +// +// indir(x) +// or +// indir(add(x, const2)) +// +// (indir is any node for which OperIsIndirOrArrLength() is true.) +// +// 2. const1 + const2 if sufficiently small. + +GenTree* Compiler::optFindNullCheckToFold(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap) +{ + assert(tree->OperIsIndirOrArrLength()); + + GenTree* addr = (tree->OperGet() == GT_ARR_LENGTH) ? tree->AsArrLen()->ArrRef() : tree->AsIndir()->Addr(); + + ssize_t offsetValue = 0; + + if ((addr->OperGet() == GT_ADD) && addr->gtGetOp2()->IsCnsIntOrI()) + { + offsetValue += addr->gtGetOp2()->AsIntConCommon()->IconValue(); + addr = addr->gtGetOp1(); + } + + if (addr->OperGet() != GT_LCL_VAR) + { + return nullptr; + } + + GenTreeLclVarCommon* const lclVarNode = addr->AsLclVarCommon(); + const unsigned ssaNum = lclVarNode->GetSsaNum(); + + if (ssaNum == SsaConfig::RESERVED_SSA_NUM) + { + return nullptr; + } + + const unsigned lclNum = lclVarNode->GetLclNum(); + GenTree* nullCheckTree = nullptr; + unsigned nullCheckLclNum = BAD_VAR_NUM; + + // Check if we saw a nullcheck on this local in this basic block + // This corresponds to nullcheck(x) tree in the header comment. + if (nullCheckMap->Lookup(lclNum, &nullCheckTree)) + { + GenTree* nullCheckAddr = nullCheckTree->AsIndir()->Addr(); + if ((nullCheckAddr->OperGet() != GT_LCL_VAR) || (nullCheckAddr->AsLclVarCommon()->GetSsaNum() != ssaNum)) + { + nullCheckTree = nullptr; + } + else + { + nullCheckLclNum = nullCheckAddr->AsLclVarCommon()->GetLclNum(); + } + } - GenTree* const addr = tree->AsIndir()->Addr(); - if (addr->OperGet() == GT_LCL_VAR) + if (nullCheckTree == nullptr) { - // Check if we have the pattern above and find the nullcheck node if we do. + // Check if we have x = comma(nullcheck(y), add(y, const1)) pattern. - // Find the definition of the indirected local (x in the picture) - GenTreeLclVarCommon* const lclVarNode = addr->AsLclVarCommon(); + // Find the definition of the indirected local ('x' in the pattern above). + LclSsaVarDsc* defLoc = lvaTable[lclNum].GetPerSsaData(ssaNum); + + if (compCurBB != defLoc->GetBlock()) + { + return nullptr; + } - const unsigned lclNum = lclVarNode->GetLclNum(); - const unsigned ssaNum = lclVarNode->GetSsaNum(); + GenTree* defRHS = defLoc->GetAssignment()->gtGetOp2(); - if (ssaNum != SsaConfig::RESERVED_SSA_NUM) + if (defRHS->OperGet() != GT_COMMA) { - LclSsaVarDsc* defLoc = lvaTable[lclNum].GetPerSsaData(ssaNum); - BasicBlock* defBlock = defLoc->GetBlock(); + return nullptr; + } + + const bool commaOnly = true; + GenTree* commaOp1EffectiveValue = defRHS->gtGetOp1()->gtEffectiveVal(commaOnly); - if (compCurBB == defBlock) + if (commaOp1EffectiveValue->OperGet() != GT_NULLCHECK) + { + return nullptr; + } + + GenTree* nullCheckAddress = commaOp1EffectiveValue->gtGetOp1(); + + if ((nullCheckAddress->OperGet() != GT_LCL_VAR) || (defRHS->gtGetOp2()->OperGet() != GT_ADD)) + { + return nullptr; + } + + // We found a candidate for 'y' in the pattern above. + + GenTree* additionNode = defRHS->gtGetOp2(); + GenTree* additionOp1 = additionNode->gtGetOp1(); + GenTree* additionOp2 = additionNode->gtGetOp2(); + if ((additionOp1->OperGet() == GT_LCL_VAR) && + (additionOp1->AsLclVarCommon()->GetLclNum() == nullCheckAddress->AsLclVarCommon()->GetLclNum()) && + (additionOp2->IsCnsIntOrI())) + { + offsetValue += additionOp2->AsIntConCommon()->IconValue(); + nullCheckTree = commaOp1EffectiveValue; + } + } + + if (fgIsBigOffset(offsetValue)) + { + return nullptr; + } + else + { + return nullCheckTree; + } +} + +//---------------------------------------------------------------- +// optIsNullCheckFoldingLegal: Check the nodes between the GT_NULLCHECK node and the indirection to determine +// if null check folding is legal. +// +// Arguments: +// tree - The input indirection tree. +// nullCheckTree - The GT_NULLCHECK tree that is a candidate for removal. +// nullCheckParent - The parent of the GT_NULLCHECK tree that is a candidate for removal (out-parameter). +// nullCheckStatement - The statement of the GT_NULLCHECK tree that is a candidate for removal (out-parameter). + +bool Compiler::optIsNullCheckFoldingLegal(GenTree* tree, + GenTree* nullCheckTree, + GenTree** nullCheckParent, + Statement** nullCheckStmt) +{ + // Check all nodes between the GT_NULLCHECK and the indirection to see + // if any nodes have unsafe side effects. + unsigned nullCheckLclNum = nullCheckTree->gtGetOp1()->AsLclVarCommon()->GetLclNum(); + bool isInsideTry = compCurBB->hasTryIndex(); + bool canRemoveNullCheck = true; + const unsigned maxNodesWalked = 50; + unsigned nodesWalked = 0; + + // First walk the nodes in the statement containing the GT_NULLCHECK in forward execution order + // until we get to the indirection or process the statement root. + GenTree* previousTree = nullCheckTree; + GenTree* currentTree = nullCheckTree->gtNext; + assert(fgStmtListThreaded); + while (canRemoveNullCheck && (currentTree != tree) && (currentTree != nullptr)) + { + if ((*nullCheckParent == nullptr) && (nullCheckTree->gtGetChildPointer(currentTree) != nullptr)) + { + *nullCheckParent = currentTree; + } + const bool checkExceptionSummary = false; + if ((nodesWalked++ > maxNodesWalked) || + !optCanMoveNullCheckPastTree(currentTree, nullCheckLclNum, isInsideTry, checkExceptionSummary)) + { + canRemoveNullCheck = false; + } + else + { + previousTree = currentTree; + currentTree = currentTree->gtNext; + } + } + + if (currentTree == tree) + { + // The GT_NULLCHECK and the indirection are in the same statements. + *nullCheckStmt = compCurStmt; + } + else + { + // The GT_NULLCHECK and the indirection are in different statements. + // Walk the nodes in the statement containing the indirection + // in reverse execution order starting with the indirection's + // predecessor. + GenTree* nullCheckStatementRoot = previousTree; + currentTree = tree->gtPrev; + while (canRemoveNullCheck && (currentTree != nullptr)) + { + const bool checkExceptionSummary = false; + if ((nodesWalked++ > maxNodesWalked) || + !optCanMoveNullCheckPastTree(currentTree, nullCheckLclNum, isInsideTry, checkExceptionSummary)) + { + canRemoveNullCheck = false; + } + else { - GenTree* defParent = defLoc->GetAssignment(); - assert(defParent->OperIs(GT_ASG)); + currentTree = currentTree->gtPrev; + } + } - if (defParent->gtNext == nullptr) - { - GenTree* defRHS = defParent->gtGetOp2(); - if (defRHS->OperGet() == GT_COMMA) - { - if (defRHS->gtGetOp1()->OperGet() == GT_NULLCHECK) - { - GenTree* nullCheckTree = defRHS->gtGetOp1(); - if (nullCheckTree->gtGetOp1()->OperGet() == GT_LCL_VAR) - { - // We found a candidate for 'y' in the picture - unsigned nullCheckLclNum = nullCheckTree->gtGetOp1()->AsLclVarCommon()->GetLclNum(); - - if (defRHS->gtGetOp2()->OperGet() == GT_ADD) - { - GenTree* additionNode = defRHS->gtGetOp2(); - if ((additionNode->gtGetOp1()->OperGet() == GT_LCL_VAR) && - (additionNode->gtGetOp1()->AsLclVarCommon()->GetLclNum() == nullCheckLclNum)) - { - GenTree* offset = additionNode->gtGetOp2(); - if (offset->IsCnsIntOrI()) - { - if (!fgIsBigOffset(offset->AsIntConCommon()->IconValue())) - { - // Walk from the use to the def in reverse execution order to see - // if any nodes have unsafe side effects. - GenTree* currentTree = lclVarNode->gtPrev; - bool isInsideTry = compCurBB->hasTryIndex(); - bool canRemoveNullCheck = true; - const unsigned maxNodesWalked = 25; - unsigned nodesWalked = 0; - - // First walk the nodes in the statement containing the indirection - // in reverse execution order starting with the indirection's - // predecessor. - while (canRemoveNullCheck && (currentTree != nullptr)) - { - if ((nodesWalked++ > maxNodesWalked) || - !optCanMoveNullCheckPastTree(currentTree, isInsideTry)) - { - canRemoveNullCheck = false; - } - else - { - currentTree = currentTree->gtPrev; - } - } - - // Then walk the statement list in reverse execution order - // until we get to the statement containing the null check. - // We only need to check the side effects at the root of each statement. - Statement* curStmt = compCurStmt->GetPrevStmt(); - currentTree = curStmt->GetRootNode(); - while (canRemoveNullCheck && (currentTree != defParent)) - { - if ((nodesWalked++ > maxNodesWalked) || - !optCanMoveNullCheckPastTree(currentTree, isInsideTry)) - { - canRemoveNullCheck = false; - } - else - { - curStmt = curStmt->GetPrevStmt(); - assert(curStmt != nullptr); - currentTree = curStmt->GetRootNode(); - } - } - - if (canRemoveNullCheck) - { - // Remove the null check - nullCheckTree->gtFlags &= ~(GTF_EXCEPT | GTF_DONT_CSE); - - // Set this flag to prevent reordering - nullCheckTree->gtFlags |= GTF_ORDER_SIDEEFF; - nullCheckTree->gtFlags |= GTF_IND_NONFAULTING; - - defRHS->gtFlags &= ~(GTF_EXCEPT | GTF_DONT_CSE); - defRHS->gtFlags |= - additionNode->gtFlags & (GTF_EXCEPT | GTF_DONT_CSE); - - // Re-morph the statement. - fgMorphBlockStmt(compCurBB, curStmt DEBUGARG("optFoldNullCheck")); - } - } - } - } - } - } - } - } - } + // Finally, walk the statement list in reverse execution order + // until we get to the statement containing the null check. + // We only check the side effects at the root of each statement. + Statement* curStmt = compCurStmt->GetPrevStmt(); + currentTree = curStmt->GetRootNode(); + while (canRemoveNullCheck && (currentTree != nullCheckStatementRoot)) + { + const bool checkExceptionSummary = true; + if ((nodesWalked++ > maxNodesWalked) || + !optCanMoveNullCheckPastTree(currentTree, nullCheckLclNum, isInsideTry, checkExceptionSummary)) + { + canRemoveNullCheck = false; + } + else + { + curStmt = curStmt->GetPrevStmt(); + currentTree = curStmt->GetRootNode(); } } + *nullCheckStmt = curStmt; } + + if (canRemoveNullCheck && (*nullCheckParent == nullptr)) + { + *nullCheckParent = nullCheckTree->gtGetParent(nullptr); + } + + return canRemoveNullCheck; } //---------------------------------------------------------------- -// optCanMoveNullCheckPastTree: Check if GT_NULLCHECK can be folded into a node that -// is after tree is execution order. +// optCanMoveNullCheckPastTree: Check if a nullcheck node that is before `tree` +// in execution order may be folded into an indirection node that +// is after `tree` is execution order. // // Arguments: -// tree - The input GT_INDIR tree. -// isInsideTry - True if tree is inside try, false otherwise +// tree - The tree to check. +// nullCheckLclNum - The local variable that GT_NULLCHECK checks. +// isInsideTry - True if tree is inside try, false otherwise. +// checkSideEffectSummary -If true, check side effect summary flags only, +// otherwise check the side effects of the operation itself. // // Return Value: -// True if GT_NULLCHECK can be folded into a node that is after tree is execution order, +// True if nullcheck may be folded into a node that is after tree in execution order, // false otherwise. -bool Compiler::optCanMoveNullCheckPastTree(GenTree* tree, bool isInsideTry) +bool Compiler::optCanMoveNullCheckPastTree(GenTree* tree, + unsigned nullCheckLclNum, + bool isInsideTry, + bool checkSideEffectSummary) { bool result = true; - if (isInsideTry) + + if ((tree->gtFlags & GTF_CALL) != 0) { - // We disallow calls, exception sources, and all assignments. - // Assignments to locals are disallowed inside try because - // they may be live in the handler. - if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0) - { - result = false; - } + result = !checkSideEffectSummary && !tree->OperRequiresCallFlag(this); } - else + + if (result && (tree->gtFlags & GTF_EXCEPT) != 0) + { + result = !checkSideEffectSummary && !tree->OperMayThrow(this); + } + + if (result && ((tree->gtFlags & GTF_ASG) != 0)) { - // We disallow calls, exception sources, and assignments to - // global memory. - if (GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS(tree->gtFlags)) + if (tree->OperGet() == GT_ASG) { - result = false; + GenTree* lhs = tree->gtGetOp1(); + GenTree* rhs = tree->gtGetOp2(); + if (checkSideEffectSummary && ((rhs->gtFlags & GTF_ASG) != 0)) + { + result = false; + } + else if (isInsideTry) + { + // Inside try we allow only assignments to locals not live in handlers. + // lvVolatileHint is set to true on variables that are line in handlers. + result = (lhs->OperGet() == GT_LCL_VAR) && !lvaTable[lhs->AsLclVarCommon()->GetLclNum()].lvVolatileHint; + } + else + { + // We disallow only assignments to global memory. + result = ((lhs->gtFlags & GTF_GLOB_REF) == 0); + } + } + else if (checkSideEffectSummary) + { + result = !isInsideTry && ((tree->gtFlags & GTF_GLOB_REF) == 0); + } + else + { + result = !isInsideTry && (!tree->OperRequiresAsgFlag() || ((tree->gtFlags & GTF_GLOB_REF) == 0)); } } + return result; } diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 132cf2bf082e4..c526e7bfb591a 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -5891,7 +5891,9 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, const B if (treeToNullcheck != nullptr) { GenTree* nullcheck = gtNewOperNode(GT_NULLCHECK, TYP_I_IMPL, treeToNullcheck); - result = gtNewOperNode(GT_COMMA, TYP_INT, nullcheck, result); + compCurBB->bbFlags |= BBF_HAS_NULLCHECK; + optMethodFlags |= OMF_HAS_NULLCHECK; + result = gtNewOperNode(GT_COMMA, TYP_INT, nullcheck, result); } impPushOnStack(result, typeInfo(TI_INT)); @@ -13017,6 +13019,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ)) { op1->ChangeOper(GT_NULLCHECK); + block->bbFlags |= BBF_HAS_NULLCHECK; + optMethodFlags |= OMF_HAS_NULLCHECK; op1->gtType = TYP_BYTE; } else @@ -15229,7 +15233,9 @@ void Compiler::impImportBlockCode(BasicBlock* block) GenTree* boxPayloadAddress = gtNewOperNode(GT_ADD, TYP_BYREF, cloneOperand, boxPayloadOffset); GenTree* nullcheck = gtNewOperNode(GT_NULLCHECK, TYP_I_IMPL, op1); - GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress); + block->bbFlags |= BBF_HAS_NULLCHECK; + optMethodFlags |= OMF_HAS_NULLCHECK; + GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress); impPushOnStack(result, tiRetVal); break; } diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 075efa426f902..c271c40450c0c 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -12560,38 +12560,47 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (op2->IsCnsIntOrI() && varTypeIsIntegralOrI(typ)) { - /* Fold "((x+icon1)+icon2) to (x+(icon1+icon2))" */ + // Fold "((x+icon1)+icon2) to (x+(icon1+icon2))" + // Fold "((comma(y, x+icon1)+icon2) to comma(y, x+(icon1+icon2))" CLANG_FORMAT_COMMENT_ANCHOR; - if (op1->gtOper == GT_ADD && // - !gtIsActiveCSE_Candidate(op1) && // - !op1->gtOverflow() && // - op1->AsOp()->gtOp2->IsCnsIntOrI() && // - (op1->AsOp()->gtOp2->OperGet() == op2->OperGet()) && // - (op1->AsOp()->gtOp2->TypeGet() != TYP_REF) && // Don't fold REFs - (op2->TypeGet() != TYP_REF)) // Don't fold REFs + const bool commasOnly = true; + GenTree* op1EffectiveValue = op1->gtEffectiveVal(commasOnly); + + if (op1EffectiveValue->gtOper == GT_ADD && !gtIsActiveCSE_Candidate(op1EffectiveValue) && + !op1EffectiveValue->gtOverflow() && op1EffectiveValue->AsOp()->gtOp2->IsCnsIntOrI() && + (op1EffectiveValue->AsOp()->gtOp2->OperGet() == op2->OperGet()) && + (op1EffectiveValue->AsOp()->gtOp2->TypeGet() != TYP_REF) && (op2->TypeGet() != TYP_REF)) { - cns1 = op1->AsOp()->gtOp2; - op2->AsIntConCommon()->SetIconValue(cns1->AsIntConCommon()->IconValue() + - op2->AsIntConCommon()->IconValue()); + cns1 = op1EffectiveValue->AsOp()->gtOp2; + + cns1->AsIntConCommon()->SetIconValue(cns1->AsIntConCommon()->IconValue() + + op2->AsIntConCommon()->IconValue()); #ifdef _TARGET_64BIT_ - if (op2->TypeGet() == TYP_INT) + if (cns1->TypeGet() == TYP_INT) { // we need to properly re-sign-extend or truncate after adding two int constants above - op2->AsIntCon()->TruncateOrSignExtend32(); + cns1->AsIntCon()->TruncateOrSignExtend32(); } #endif //_TARGET_64BIT_ if (cns1->OperGet() == GT_CNS_INT) { - op2->AsIntCon()->gtFieldSeq = + cns1->AsIntCon()->gtFieldSeq = GetFieldSeqStore()->Append(cns1->AsIntCon()->gtFieldSeq, op2->AsIntCon()->gtFieldSeq); } - DEBUG_DESTROY_NODE(cns1); + DEBUG_DESTROY_NODE(op2); - tree->AsOp()->gtOp1 = op1->AsOp()->gtOp1; - DEBUG_DESTROY_NODE(op1); - op1 = tree->AsOp()->gtOp1; + GenTree* oldTree = tree; + tree = tree->AsOp()->gtOp1; + op1 = tree->AsOp()->gtOp1; + op2 = tree->AsOp()->gtOp2; + DEBUG_DESTROY_NODE(oldTree); + + if (tree->OperGet() != GT_ADD) + { + return tree; + } } // Fold (x + 0).