Improvements for null check folding. (#1735)

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

@@ -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
+                     {

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)
     {

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);

src/coreclr/src/jit/compiler.h

@@ -6396,17 +6396,27 @@ class Compiler
         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
     /**************************************************************************

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