Fix 1176647: Jit does invalid reordering of left and right side

This bug occurs in the Lowering of the code for a compare, where op1 is an indirection, and op2 is a call.  It chooses to make op1 "contained" (i.e. do the load as part of the compare op), but this is invalid because the call may modify op1.

This also fixes a similar issue with RMW operators (e.g. +=).

Fixing this causes numerous regressions (a few improvements, due to keeping fewer things live across the call), but they look valid - that is, they are a similar case of delaying a load past a call.

[tfs-changeset: 1480705]

diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp
index 4327e20..3186495 100644 (file)
--- a/src/jit/lower.cpp
+++ b/src/jit/lower.cpp
@@ -72,6 +72,53 @@ bool Lowering::CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNod
 }
 
 //------------------------------------------------------------------------
+// IsSafeToContainMem: Checks for conflicts between childNode and parentNode.
+//
+// Arguments:
+//    parentNode  - a non-leaf binary node
+//    childNode   - a memory op that is a child op of 'parentNode'
+//
+// Return value:
+//    returns true if it is safe to make childNode a contained memory op
+//
+// Notes:
+//    Checks for memory conflicts in the instructions between childNode and parentNode,
+//    and returns true iff childNode can be contained.
+
+bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode)
+{
+    assert(parentNode->OperIsBinary());
+    assert(childNode->isMemoryOp());
+
+    // Check conflicts against nodes between 'childNode' and 'parentNode'
+    GenTree* node;
+    unsigned int childFlags = (childNode->gtFlags & GTF_ALL_EFFECT);
+    for (node = childNode->gtNext;
+         (node != parentNode) && (node != nullptr);
+         node = node->gtNext)
+    {
+        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))
+        {
+            return false;
+        }
+    }
+    if (node != parentNode)
+    {
+        assert(!"Ran off end of stmt\n");
+        return false;
+    }
+    return true;
+}
+
+//------------------------------------------------------------------------
 
 Compiler::fgWalkResult Lowering::DecompNodeHelper(GenTreePtr* pTree, Compiler::fgWalkData* data)
 {

diff --git a/src/jit/lower.h b/src/jit/lower.h
index e029e45..ae1f73e 100644 (file)
--- a/src/jit/lower.h
+++ b/src/jit/lower.h
@@ -189,6 +189,9 @@ private:
     // Checks and makes 'childNode' contained in the 'parentNode'
     bool CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNode);
     
+    // Checks for memory conflicts in the instructions between childNode and parentNode, and returns true if childNode can be contained.
+    bool IsSafeToContainMem(GenTree* parentNode, GenTree* childNode);
+
     LinearScan *m_lsra;
     BasicBlock *currBlock;
     unsigned vtableCallTemp; // local variable we use as a temp for vtable calls

diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp
index d803e5d..08c340c 100644 (file)
--- a/src/jit/lowerxarch.cpp
+++ b/src/jit/lowerxarch.cpp
@@ -2235,10 +2235,17 @@ void Lowering::LowerCmp(GenTreePtr tree)
         }
 
         assert(otherOp != nullptr);
-        if (otherOp->isMemoryOp() || otherOp->IsCnsNonZeroFltOrDbl())
+        if (otherOp->IsCnsNonZeroFltOrDbl())
         {
             MakeSrcContained(tree, otherOp);
         }
+        else if (otherOp->isMemoryOp())
+        {
+            if ((otherOp == op2) || IsSafeToContainMem(tree, otherOp)) 
+            {
+                MakeSrcContained(tree, otherOp);
+            }
+        }
 
         return;
     }
@@ -2467,11 +2474,11 @@ void Lowering::LowerCmp(GenTreePtr tree)
             }
         }
     }
-    else if (op1->isMemoryOp()) 
+    else if (op2->isMemoryOp())
     {
-        if(op1Type == op2Type)
+        if (op1Type == op2Type)
         {
-            MakeSrcContained(tree, op1);
+            MakeSrcContained(tree, op2);
 
             // Mark the tree as doing unsigned comparison if
             // both the operands are small and unsigned types.
@@ -2484,11 +2491,11 @@ void Lowering::LowerCmp(GenTreePtr tree)
             }
         }
     }
-    else if (op2->isMemoryOp())
+    else if (op1->isMemoryOp()) 
     {
-        if(op1Type == op2Type)
+        if ((op1Type == op2Type) && IsSafeToContainMem(tree, op1))
         {
-            MakeSrcContained(tree, op2);
+            MakeSrcContained(tree, op1);
 
             // Mark the tree as doing unsigned comparison if
             // both the operands are small and unsigned types.
@@ -2699,7 +2706,8 @@ bool Lowering::LowerStoreInd(GenTreePtr tree)
         GenTreePtr indirOpSource = nullptr;
 
         if (rhsLeft->OperGet() == GT_IND &&
-            rhsLeft->gtGetOp1()->OperGet() == indirDst->OperGet())
+            rhsLeft->gtGetOp1()->OperGet() == indirDst->OperGet() &&
+            IsSafeToContainMem(indirSrc, rhsLeft))
         {
             indirCandidate = rhsLeft;
             indirOpSource = rhsRight;
@@ -2922,7 +2930,10 @@ void Lowering::SetMulOpCounts(GenTreePtr tree)
     // To generate an LEA we need to force memOp into a register
     // so don't allow memOp to be 'contained'
     //
-    if ((memOp != nullptr) && !useLeaEncoding && (memOp->TypeGet() == tree->TypeGet()))
+    if ((memOp != nullptr)                    &&
+        !useLeaEncoding                       &&
+        (memOp->TypeGet() == tree->TypeGet()) &&
+        IsSafeToContainMem(tree, memOp))
     {
         MakeSrcContained(tree, memOp);
     }
@@ -2935,7 +2946,10 @@ void Lowering::SetMulOpCounts(GenTreePtr tree)
 //    tree      - a binary tree node
 //
 // Return Value:
-//    returns true if we can use the read-modify-write memory instruction form
+//    Returns true if we can use the read-modify-write instruction form
+//
+// Notes:
+//    This is used to determine whether to preference the source to the destination register.
 //
 bool Lowering::isRMWRegOper(GenTreePtr tree)
 {