Merge pull request #7410 from pgavlin/gh7380-1.1

Fix lowering's containment analysis.

Author: RussKeldorph

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"