Improvements for null check folding. (#1735)
authorEugene Rozenfeld <erozen@microsoft.com>
Fri, 17 Jan 2020 23:35:53 +0000 (15:35 -0800)
committerGitHub <noreply@github.com>
Fri, 17 Jan 2020 23:35:53 +0000 (15:35 -0800)
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 [new file with mode: 0644]
src/coreclr/src/jit/block.cpp
src/coreclr/src/jit/block.h
src/coreclr/src/jit/compiler.h
src/coreclr/src/jit/compmemkind.h
src/coreclr/src/jit/earlyprop.cpp
src/coreclr/src/jit/importer.cpp
src/coreclr/src/jit/morph.cpp

diff --git a/src/coreclr/format.patch b/src/coreclr/format.patch
new file mode 100644 (file)
index 0000000..3759a98
--- /dev/null
@@ -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
+                     {
index 62242d8..c44b262 100644 (file)
@@ -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)
     {
index c4ef847..79e11ed 100644 (file)
@@ -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);
index 23c692e..938b717 100644 (file)
@@ -6396,17 +6396,27 @@ public:
         OPK_NULLCHECK
     };
 
+    typedef JitHashTable<unsigned, JitSmallPrimitiveKeyFuncs<unsigned>, 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
     /**************************************************************************
index 986c4e2..f680b04 100644 (file)
@@ -57,6 +57,7 @@ CompMemKindMacro(ObjectAllocator)
 CompMemKindMacro(VariableLiveRanges)
 CompMemKindMacro(ClassLayout)
 CompMemKindMacro(TailMergeThrows)
+CompMemKindMacro(EarlyProp)
 //clang-format on
 
 #undef CompMemKindMacro
index 9d06857..64f6e98 100644 (file)
@@ -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;
 }
index 132cf2b..c526e7b 100644 (file)
@@ -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;
                             }
index 075efa4..c271c40 100644 (file)
@@ -12560,38 +12560,47 @@ DONE_MORPHING_CHILDREN:
 
                 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).