Fix lowering's containment analysis.

This fixes a silent bad code generation issue that arose during internal
testing. The original repro is a test failure under COMPlus_JitStress=2.
Due to explicit null check insertion, we (eventually) end up with the
following LIR:

    t6096 =    lclVar    ref    V86 cse10         <l:$4ad, c:$1b5>

            /--*  t6096  ref
            *  st.lclVar ref    V41 tmp29        d:26

    t2733 =    lclVar    ref    V41 tmp29        u:26 <l:$4ad, c:$1b5>

            /--*  t2733  ref
            *  nullcheck byte   <l:$4b8, c:$58a>

    t2736 =    lclVar    ref    V41 tmp29        u:26 (last use) <l:$4ad, c:$1b5>

    t2737 =    const     long   20 field offset Fseq[y] $107

            /--*  t2736  ref
            +--*  t2737  long
    t2735 = *  +         byref  <l:$2ad, c:$2ac>

    t6081 =    lclVar    ref    V83 cse7          <l:$4bd, c:$1b7>

            /--*  t6081  ref
            *  st.lclVar ref    V41 tmp29        d:27

    t2762 =    lclVar    ref    V41 tmp29        u:27 <l:$4bd, c:$1b7>

            /--*  t2762  ref
            *  nullcheck byte   <l:$583, c:$58f>

    t2765 =    lclVar    ref    V41 tmp29        u:27 (last use) <l:$4bd, c:$1b7>

    t2766 =    const     long   20 field offset Fseq[y] $107

            /--*  t2765  ref
            +--*  t2766  long
    t2764 = *  +         byref  <l:$2af, c:$2ae>

            /--*  t2764  byref
    t2763 = *  indir     int    <l:$54e, c:$1ed>

    t2767 =    lclVar    int   (AX) V07 loc4          $1ee

            /--*  t2763  int
            +--*  t2767  int
    t2738 = *  +         int    <l:$554, c:$553>

            /--*  t2735  byref
            +--*  t2738  int
            *  storeIndir int

During lowering, we attempt to form an RMW add rooted at the final
storeIndir. The pattern matching that attempts to form RMW operations,
however, does not consider whether or not it is safe to perform the
code motion involved in making the destination and source addresses
for the operator contained. In this case, lowering moves the evaluation
of the address (i.e. the dataflow tree rooted at the add that produces
t2735) into the storeIndir. This moves a use of tmp29 across a def of
the same and causes the program to store a value to an incorrect
address.

There are many variations on this pattern. For example, given the
following C#:

	static int T(C[] a, C c)
	{
	    return a.Length != c.M() ? 100 : 0;
	}

The evaluation of a.Length (including the necessary null check) should
occur before the call to c.M(). The lack of correct checks for safe
code motion that caused the original repro, however, cause the JIT to
generate bad code in this case as well: the null check for a is folded
into the load of a.Length, which is then made contained by the compare.
This results in the call to c.M() executing before the null check, which
causes the program to behave incorrectly in the case that a is null.

In order to fix the code motion analysis, this change introduces a new
type, `SideEffectSet`, that can be used to summarize the side effects
of a set of nodes and check whether or not they interfere with another
set of side effects. This change then uses the new type to ensure that
it is safe to perform the code motion necessary to make an operand
contained before doing so.

Author: pgavlin

src/jit/CMakeLists.txt

@@ -63,6 +63,7 @@ set( JIT_SOURCES
   regset.cpp
   scopeinfo.cpp
   sharedfloat.cpp
+  sideeffects.cpp
   sm.cpp
   smdata.cpp
   smweights.cpp

src/jit/codegenxarch.cpp

@@ -5133,13 +5133,13 @@ regNumber CodeGen::genConsumeReg(GenTree* tree)
 // Do liveness update for an address tree: one of GT_LEA, GT_LCL_VAR, or GT_CNS_INT (for call indirect).
 void CodeGen::genConsumeAddress(GenTree* addr)
 {
-    if (addr->OperGet() == GT_LEA)
+    if (!addr->isContained())
     {
-        genConsumeAddrMode(addr->AsAddrMode());
+        genConsumeReg(addr);
     }
-    else if (!addr->isContained())
+    else if (addr->OperGet() == GT_LEA)
     {
-        genConsumeReg(addr);
+        genConsumeAddrMode(addr->AsAddrMode());
     }
 }
 

src/jit/gentree.cpp

@@ -1607,6 +1607,30 @@ regMaskTP GenTreeCall::GetOtherRegMask() const
     return resultMask;
 }
 
+//-------------------------------------------------------------------------
+// IsPure:
+//    Returns true if this call is pure. For now, this uses the same
+//    definition of "pure" that is that used by HelperCallProperties: a
+//    pure call does not read or write any aliased (e.g. heap) memory or
+//    have other global side effects (e.g. class constructors, finalizers),
+//    but is allowed to throw an exception.
+//
+//    NOTE: this call currently only returns true if the call target is a
+//    helper method that is known to be pure. No other analysis is
+//    performed.
+//
+// Arguments:
+//    Copiler - the compiler context.
+//
+// Returns:
+//    True if the call is pure; false otherwise.
+//
+bool GenTreeCall::IsPure(Compiler* compiler) const
+{
+    return (gtCallType == CT_HELPER) &&
+           compiler->s_helperCallProperties.IsPure(compiler->eeGetHelperNum(gtCallMethHnd));
+}
+
 #ifndef LEGACY_BACKEND
 
 //-------------------------------------------------------------------------

src/jit/gentree.h

@@ -1365,6 +1365,7 @@ struct GenTree
         {
             case GT_LOCKADD:
             case GT_XADD:
+            case GT_XCHG:
             case GT_CMPXCHG:
             case GT_BLK:
             case GT_OBJ:
@@ -1404,7 +1405,7 @@ struct GenTree
         return (gtOper == GT_XADD || gtOper == GT_XCHG || gtOper == GT_LOCKADD || gtOper == GT_CMPXCHG);
     }
 
-    bool OperIsAtomicOp()
+    bool OperIsAtomicOp() const
     {
         return OperIsAtomicOp(gtOper);
     }
@@ -3306,6 +3307,8 @@ struct GenTreeCall final : public GenTree
         return (gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0;
     }
 
+    bool IsPure(Compiler* compiler) const;
+
     unsigned short gtCallMoreFlags; // in addition to gtFlags
 
     unsigned char gtCallType : 3;   // value from the gtCallTypes enumeration

src/jit/hashbv.cpp

@@ -289,6 +289,19 @@ elemType hashBvNode::SubtractWithChange(hashBvNode* other)
     return result;
 }
 
+bool hashBvNode::Intersects(hashBvNode* other)
+{
+    for (int i = 0; i < this->numElements(); i++)
+    {
+        if ((this->elements[i] & other->elements[i]) != 0)
+        {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 void hashBvNode::AndWith(hashBvNode* other)
 {
     for (int i = 0; i < this->numElements(); i++)
@@ -1234,6 +1247,46 @@ class CompareAction
     }
 };
 
+class IntersectsAction
+{
+public:
+    static inline void PreAction(hashBv* lhs, hashBv* rhs)
+    {
+    }
+    static inline void PostAction(hashBv* lhs, hashBv* rhs)
+    {
+    }
+    static inline bool DefaultResult()
+    {
+        return false;
+    }
+
+    static inline void LeftGap(hashBv* lhs, hashBvNode**& l, hashBvNode*& r, bool& result, bool& terminate)
+    {
+        // in rhs, not lhs
+        // so skip rhs
+        r = r->next;
+    }
+    static inline void RightGap(hashBv* lhs, hashBvNode**& l, hashBvNode*& r, bool& result, bool& terminate)
+    {
+        // in lhs, not rhs
+        // so skip lhs
+        l = &((*l)->next);
+    }
+    static inline void BothPresent(hashBv* lhs, hashBvNode**& l, hashBvNode*& r, bool& result, bool& terminate)
+    {
+        if ((*l)->Intersects(r))
+        {
+            terminate = true;
+            result    = true;
+        }
+    }
+    static inline void LeftEmpty(hashBv* lhs, hashBvNode**& l, hashBvNode*& r, bool& result, bool& terminate)
+    {
+        r = r->next;
+    }
+};
+
 template <typename Action>
 bool hashBv::MultiTraverseLHSBigger(hashBv* other)
 {
@@ -1507,6 +1560,11 @@ bool hashBv::MultiTraverse(hashBv* other)
     }
 }
 
+bool hashBv::Intersects(hashBv* other)
+{
+    return MultiTraverse<IntersectsAction>(other);
+}
+
 bool hashBv::AndWithChange(hashBv* other)
 {
     return MultiTraverse<AndAction>(other);

src/jit/hashbv.h

@@ -157,6 +157,8 @@ class hashBvNode
     elemType XorWithChange(hashBvNode* other);
     elemType SubtractWithChange(hashBvNode* other);
 
+    bool Intersects(hashBvNode* other);
+
 #ifdef DEBUG
     void dump();
 #endif // DEBUG
@@ -253,6 +255,8 @@ class hashBv
     bool XorWithChange(hashBv* other);
     bool SubtractWithChange(hashBv* other);
 
+    bool Intersects(hashBv* other);
+
     template <class Action>
     bool MultiTraverseLHSBigger(hashBv* other);
     template <class Action>

src/jit/jit.settings.targets

@@ -86,6 +86,7 @@
         <CppCompile Include="..\jitconfig.cpp" />
         <CppCompile Include="..\hostallocator.cpp" />
         <CppCompile Include="..\objectalloc.cpp" />
+        <CppCompile Inlcude="..\sideeffects.cpp" />
         <CppCompile Condition="'$(ClDefines.Contains(`LEGACY_BACKEND`))'=='True'" Include="..\CodeGenLegacy.cpp" />
         <CppCompile Condition="'$(ClDefines.Contains(`LEGACY_BACKEND`))'=='False'"  Include="..\Lower.cpp" />
         <CppCompile Condition="'$(ClDefines.Contains(`LEGACY_BACKEND`))'=='False'"  Include="..\LSRA.cpp" />

src/jit/lower.cpp

@@ -78,34 +78,20 @@ bool Lowering::CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNod
 // and returns 'true' iff memory operand childNode can be contained in parentNode.
 //
 // Arguments:
-//    parentNode  - a non-leaf binary node
-//    childNode   - a memory op that is a child op of 'parentNode'
+//    parentNode - any non-leaf node
+//    childNode  - some node that is an input to `parentNode`
 //
 // Return value:
 //    true if it is safe to make childNode a contained memory operand.
 //
 bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode)
 {
-    assert(parentNode->OperIsBinary());
-    assert(childNode->isMemoryOp());
+    m_scratchSideEffects.Clear();
+    m_scratchSideEffects.AddNode(comp, childNode);
 
-    unsigned int childFlags = (childNode->gtFlags & GTF_ALL_EFFECT);
-
-    GenTree* node;
-    for (node = childNode; node != parentNode; node = node->gtNext)
+    for (GenTree* node = childNode->gtNext; node != parentNode; node = node->gtNext)
     {
-        assert(node != nullptr);
-
-        if ((childFlags != 0) && node->IsCall())
-        {
-            bool isPureHelper = (node->gtCall.gtCallType == CT_HELPER) &&
-                                comp->s_helperCallProperties.IsPure(comp->eeGetHelperNum(node->gtCall.gtCallMethHnd));
-            if (!isPureHelper && ((node->gtFlags & childFlags & GTF_ALL_EFFECT) != 0))
-            {
-                return false;
-            }
-        }
-        else if (node->OperIsStore() && comp->fgNodesMayInterfere(node, childNode))
+        if (m_scratchSideEffects.InterferesWith(comp, node, false))
         {
             return false;
         }
@@ -2978,17 +2964,57 @@ void Lowering::AddrModeCleanupHelper(GenTreeAddrMode* addrMode, GenTree* node)
     BlockRange().Remove(node);
 }
 
-// given two nodes which will be used in an addressing mode (base, index)
-// walk backwards from the use to those nodes to determine if they are
-// potentially modified in that range
+//------------------------------------------------------------------------
+// Lowering::AreSourcesPossibleModifiedLocals:
+//    Given two nodes which will be used in an addressing mode (base,
+//    index), check to see if they are lclVar reads, and if so, walk
+//    backwards from the use until both reads have been visited to
+//    determine if they are potentially modified in that range.
+//
+// Arguments:
+//    addr - the node that uses the base and index nodes
+//    base - the base node
+//    index - the index node
+//
+// Returns: true if either the base or index may be modified between the
+//          node and addr.
 //
-// returns: true if the sources given may be modified before they are used
-bool Lowering::AreSourcesPossiblyModified(GenTree* addr, GenTree* base, GenTree* index)
+bool Lowering::AreSourcesPossiblyModifiedLocals(GenTree* addr, GenTree* base, GenTree* index)
 {
     assert(addr != nullptr);
 
-    for (GenTree* cursor = addr; cursor != nullptr; cursor = cursor->gtPrev)
+    unsigned markCount = 0;
+
+    SideEffectSet baseSideEffects;
+    if (base != nullptr)
     {
+        if (base->OperIsLocalRead())
+        {
+            baseSideEffects.AddNode(comp, base);
+        }
+        else
+        {
+            base = nullptr;
+        }
+    }
+
+    SideEffectSet indexSideEffects;
+    if (index != nullptr)
+    {
+        if (index->OperIsLocalRead())
+        {
+            indexSideEffects.AddNode(comp, index);
+        }
+        else
+        {
+            index = nullptr;
+        }
+    }
+
+    for (GenTree* cursor = addr;; cursor = cursor->gtPrev)
+    {
+        assert(cursor != nullptr);
+
         if (cursor == base)
         {
             base = nullptr;
@@ -2999,17 +3025,19 @@ bool Lowering::AreSourcesPossiblyModified(GenTree* addr, GenTree* base, GenTree*
             index = nullptr;
         }
 
-        if (base == nullptr && index == nullptr)
+        if ((base == nullptr) && (index == nullptr))
         {
             return false;
         }
 
-        if (base != nullptr && comp->fgNodesMayInterfere(base, cursor))
+        m_scratchSideEffects.Clear();
+        m_scratchSideEffects.AddNode(comp, cursor);
+        if ((base != nullptr) && m_scratchSideEffects.InterferesWith(baseSideEffects, false))
         {
             return true;
         }
 
-        if (index != nullptr && comp->fgNodesMayInterfere(index, cursor))
+        if ((index != nullptr) && m_scratchSideEffects.InterferesWith(indexSideEffects, false))
         {
             return true;
         }
@@ -3092,7 +3120,7 @@ GenTree* Lowering::TryCreateAddrMode(LIR::Use&& use, bool isIndir)
     }
 
     // make sure there are not any side effects between def of leaves and use
-    if (!doAddrMode || AreSourcesPossiblyModified(addr, base, index))
+    if (!doAddrMode || AreSourcesPossiblyModifiedLocals(addr, base, index))
     {
         JITDUMP("  No addressing mode\n");
         return addr;
@@ -3122,7 +3150,8 @@ GenTree* Lowering::TryCreateAddrMode(LIR::Use&& use, bool isIndir)
     GenTreeAddrMode* addrMode = new (comp, GT_LEA) GenTreeAddrMode(addrModeType, base, index, scale, offset);
 
     addrMode->gtRsvdRegs = addr->gtRsvdRegs;
-    addrMode->gtFlags |= (addr->gtFlags & (GTF_ALL_EFFECT | GTF_IND_FLAGS));
+    addrMode->gtFlags |= (addr->gtFlags & GTF_IND_FLAGS);
+    addrMode->gtFlags &= ~GTF_ALL_EFFECT; // LEAs are side-effect-free.
 
     JITDUMP("New addressing mode node:\n");
     DISPNODE(addrMode);

src/jit/lower.h

@@ -17,6 +17,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 #include "compiler.h"
 #include "phase.h"
 #include "lsra.h"
+#include "sideeffects.h"
 
 class Lowering : public Phase
 {
@@ -228,6 +229,7 @@ class Lowering : public Phase
     void LowerCmp(GenTreePtr tree);
 
 #if !CPU_LOAD_STORE_ARCH
+    bool IsRMWIndirCandidate(GenTree* operand, GenTree* storeInd);
     bool IsBinOpInRMWStoreInd(GenTreePtr tree);
     bool IsRMWMemOpRootedAtStoreInd(GenTreePtr storeIndTree, GenTreePtr* indirCandidate, GenTreePtr* indirOpSource);
     bool SetStoreIndOpCountsIfRMWMemOp(GenTreePtr storeInd);
@@ -247,7 +249,7 @@ class Lowering : public Phase
 private:
     static bool NodesAreEquivalentLeaves(GenTreePtr candidate, GenTreePtr storeInd);
 
-    bool AreSourcesPossiblyModified(GenTree* addr, GenTree* base, GenTree* index);
+    bool AreSourcesPossiblyModifiedLocals(GenTree* addr, GenTree* base, GenTree* index);
 
     // return true if 'childNode' is an immediate that can be contained
     //  by the 'parentNode' (i.e. folded into an instruction)
@@ -269,9 +271,10 @@ class Lowering : public Phase
         return LIR::AsRange(m_block);
     }
 
-    LinearScan* m_lsra;
-    unsigned    vtableCallTemp; // local variable we use as a temp for vtable calls
-    BasicBlock* m_block;
+    LinearScan*   m_lsra;
+    unsigned      vtableCallTemp;       // local variable we use as a temp for vtable calls
+    SideEffectSet m_scratchSideEffects; // SideEffectSet used for IsSafeToContainMem and isRMWIndirCandidate
+    BasicBlock*   m_block;
 };
 
 #endif // _LOWER_H_

src/jit/lowerarm.cpp

@@ -28,6 +28,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 #ifdef _TARGET_ARM_
 
 #include "jit.h"
+#include "sideeffects.h"
 #include "lower.h"
 #include "lsra.h"