Refactor call arg table updates.
authorPat Gavlin <pagavlin@microsoft.com>
Tue, 13 Sep 2016 20:42:14 +0000 (13:42 -0700)
committerPat Gavlin <pagavlin@microsoft.com>
Thu, 15 Sep 2016 23:25:13 +0000 (16:25 -0700)
When a call argument is replaced by a new node, the corresponding entry
in the call's argument table must be replaced. Managing this replacement
was a bit ad-hoc: there were a (small) number of places throughout the
compiler that needed to do so, and each determined whether or not to
call the udpate method (`fgFixupArgTabEntryPtr`) independently. The
update method has been removed and its functionality replaced with a
new method, `GenTree::ReplaceOperand`, which will update the call
argument table iff the replaced node is a call argument.

Commit migrated from https://github.com/dotnet/coreclr/commit/8ebb1d54583d0ebc03235e78e51f3026fbb7e4d3

src/coreclr/src/jit/compiler.h
src/coreclr/src/jit/gentree.cpp
src/coreclr/src/jit/gentree.h
src/coreclr/src/jit/lir.cpp
src/coreclr/src/jit/lower.cpp
src/coreclr/src/jit/rationalize.cpp

index 50cc3f9..a3a20a2 100644 (file)
@@ -4381,13 +4381,6 @@ private:
     GenTree* fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
     GenTree* fgMakeMultiUse(GenTree** ppTree);
 
-    //                  After replacing oldChild with newChild, fixup the fgArgTabEntryPtr
-    //                  if it happens to be an argument to a call.
-    void fgFixupIfCallArg(ArrayStack<GenTree*>* parentStack, GenTree* oldChild, GenTree* newChild);
-
-public:
-    void fgFixupArgTabEntryPtr(GenTreePtr parentCall, GenTreePtr oldArg, GenTreePtr newArg);
-
 private:
     //                  Recognize a bitwise rotation pattern and convert into a GT_ROL or a GT_ROR node.
     GenTreePtr fgRecognizeAndMorphBitwiseRotation(GenTreePtr tree);
index 8df171f..3bea668 100644 (file)
@@ -1740,6 +1740,44 @@ bool GenTreeCall::IsHelperCall(Compiler* compiler, unsigned helper) const
     return IsHelperCall(compiler->eeFindHelper(helper));
 }
 
+//------------------------------------------------------------------------
+// GenTreeCall::ReplaceCallOperand:
+//    Replaces a given operand to a call node and updates the call
+//    argument table if necessary.
+//
+// Arguments:
+//    useEdge - the use edge that points to the operand to be replaced.
+//    replacement - the replacement node.
+//
+void GenTreeCall::ReplaceCallOperand(GenTree** useEdge, GenTree* replacement)
+{
+    assert(useEdge != nullptr);
+    assert(replacement != nullptr);
+    assert(TryGetUse(*useEdge, &useEdge));
+
+    GenTree* originalOperand = *useEdge;
+    *useEdge = replacement;
+
+    const bool isArgument = (replacement != gtControlExpr) &&
+        ((gtCallType != CT_INDIRECT) || ((replacement != gtCallCookie) && (replacement != gtCallAddr)));
+
+    if (isArgument)
+    {
+        if ((originalOperand->gtFlags & GTF_LATE_ARG) != 0)
+        {
+            replacement->gtFlags |= GTF_LATE_ARG;
+        }
+        else
+        {
+            assert((replacement->gtFlags & GTF_LATE_ARG) == 0);
+
+            fgArgTabEntryPtr fp = Compiler::gtArgEntryByNode(this, originalOperand);
+            assert(fp->node == originalOperand);
+            fp->node = replacement;
+        }
+    }
+}
+
 /*****************************************************************************
  *
  *  Returns non-zero if the two trees are identical.
@@ -6003,6 +6041,9 @@ GenTreePtr* GenTree::gtGetChildPointer(GenTreePtr parent)
 
 bool GenTree::TryGetUse(GenTree* def, GenTree*** use)
 {
+    assert(def != nullptr);
+    assert(use != nullptr);
+
     for (GenTree** useEdge : UseEdges())
     {
         if (*useEdge == def)
@@ -6016,6 +6057,32 @@ bool GenTree::TryGetUse(GenTree* def, GenTree*** use)
 }
 
 //------------------------------------------------------------------------
+// GenTree::ReplaceOperand:
+//    Replace a given operand to this node with a new operand. If the
+//    current node is a call node, this will also udpate the call
+//    argument table if necessary.
+//
+// Arguments:
+//    useEdge - the use edge that points to the operand to be replaced.
+//    replacement - the replacement node.
+//
+void GenTree::ReplaceOperand(GenTree** useEdge, GenTree* replacement)
+{
+    assert(useEdge != nullptr);
+    assert(replacement != nullptr);
+    assert(TryGetUse(*useEdge, &useEdge));
+
+    if (OperGet() == GT_CALL)
+    {
+        AsCall()->ReplaceCallOperand(useEdge, replacement);
+    }
+    else
+    {
+        *useEdge = replacement;
+    }
+}
+
+//------------------------------------------------------------------------
 // gtGetParent: Get the parent of this node, and optionally capture the
 //    pointer to the child so that it can be modified.
 //
@@ -8101,12 +8168,31 @@ GenTreePtr Compiler::gtReplaceTree(GenTreePtr stmt, GenTreePtr tree, GenTreePtr
     {
         assert(treeParent != nullptr);
 
+        // Check to see if the node to be replaced is a call argument and if so,
+        // set `treeParent` to the call node.
+        GenTree* cursor = treeParent;
+        while ((cursor != nullptr) && (cursor->OperGet() == GT_LIST))
+        {
+            cursor = cursor->gtNext;
+        }
+
+        if ((cursor != nullptr) && (cursor->OperGet() == GT_CALL))
+        {
+            treeParent = cursor;
+        }
+
+#ifdef DEBUG
+        GenTree** useEdge;
+        assert(treeParent->TryGetUse(tree, &useEdge));
+        assert(useEdge == treePtr);
+#endif // DEBUG
+
         GenTreePtr treeFirstNode = fgGetFirstNode(tree);
         GenTreePtr treeLastNode  = tree;
         GenTreePtr treePrevNode  = treeFirstNode->gtPrev;
         GenTreePtr treeNextNode  = treeLastNode->gtNext;
 
-        *treePtr = replacementTree;
+        treeParent->ReplaceOperand(treePtr, replacementTree);
 
         // Build the linear order for "replacementTree".
         fgSetTreeSeq(replacementTree, treePrevNode);
@@ -8133,48 +8219,6 @@ GenTreePtr Compiler::gtReplaceTree(GenTreePtr stmt, GenTreePtr tree, GenTreePtr
             treeNextNode->gtPrev = treeLastNode;
         }
 
-        bool       needFixupCallArg = false;
-        GenTreePtr node             = treeParent;
-
-        // If we have replaced an arg, then update pointers in argtable.
-        do
-        {
-            // Look for the first enclosing callsite
-            switch (node->OperGet())
-            {
-                case GT_LIST:
-                case GT_ARGPLACE:
-                    // "tree" is likely an argument of a call.
-                    needFixupCallArg = true;
-                    break;
-
-                case GT_CALL:
-                    if (needFixupCallArg)
-                    {
-                        // We have replaced an arg, so update pointers in argtable.
-                        fgFixupArgTabEntryPtr(node, tree, replacementTree);
-                        needFixupCallArg = false;
-                    }
-                    break;
-
-                default:
-                    // "tree" is unlikely an argument of a call.
-                    needFixupCallArg = false;
-                    break;
-            }
-
-            if (needFixupCallArg)
-            {
-                // Keep tracking to update the first enclosing call.
-                node = node->gtGetParent(nullptr);
-            }
-            else
-            {
-                // Stop tracking.
-                node = nullptr;
-            }
-        } while (node != nullptr);
-
         // Propagate side-effect flags of "replacementTree" to its parents if needed.
         gtUpdateSideEffects(treeParent, tree->gtFlags, replacementTree->gtFlags);
     }
index 53aca89..5716cae 100644 (file)
@@ -1511,6 +1511,8 @@ public:
     // Get the parent of this node, and optionally capture the pointer to the child so that it can be modified.
     GenTreePtr gtGetParent(GenTreePtr** parentChildPtrPtr);
 
+    void ReplaceOperand(GenTree** useEdge, GenTree* replacement);
+
     inline GenTreePtr gtEffectiveVal(bool commaOnly = false);
 
     // Return the child of this node if it is a GT_RELOAD or GT_COPY; otherwise simply return the node itself
@@ -3361,6 +3363,8 @@ struct GenTreeCall final : public GenTree
 
     bool IsHelperCall(Compiler* compiler, unsigned helper) const;
 
+    void ReplaceCallOperand(GenTree** operandUseEdge, GenTree* replacement);
+
     GenTreeCall(var_types type) : GenTree(GT_CALL, type)
     {
     }
index ed39dce..cd7856f 100644 (file)
@@ -190,12 +190,9 @@ void LIR::Use::ReplaceWith(Compiler* compiler, GenTree* replacement)
     assert(IsDummyUse() || m_range->Contains(m_user));
     assert(m_range->Contains(replacement));
 
-    GenTree* replacedNode = *m_edge;
-
-    *m_edge = replacement;
-    if (!IsDummyUse() && m_user->IsCall())
+    if (!IsDummyUse())
     {
-        compiler->fgFixupArgTabEntryPtr(m_user, replacedNode, replacement);
+        m_user->ReplaceOperand(m_edge, replacement);
     }
 }
 
index e059407..61bc6c9 100644 (file)
@@ -3314,9 +3314,9 @@ GenTree* Lowering::TryCreateAddrMode(LIR::Use&& use, bool isIndir)
         {
             // We can have an indirection on the rhs of a block copy (it is the source
             // object). This is not a "regular" indirection.
-            // (Note that the parent check could be costly.)
-            GenTree* parent = indir->gtGetParent(nullptr);
-            if ((parent != nullptr) && parent->OperIsIndir())
+            // (Note that the user check could be costly.)
+            LIR::Use indirUse;
+            if (BlockRange().TryGetUse(indir, &indirUse) && indirUse.User()->OperIsIndir())
             {
                 isIndir = false;
             }
index 4a204c2..081d642 100644 (file)
@@ -16,44 +16,6 @@ struct SplitData
     Rationalizer* thisPhase;
 };
 
-//------------------------------------------------------------------------------
-// isNodeCallArg - given a context (stack of parent nodes), determine if the TOS is an arg to a call
-//------------------------------------------------------------------------------
-
-GenTree* isNodeCallArg(ArrayStack<GenTree*>* parentStack)
-{
-    for (int i = 1; // 0 is current node, so start at 1
-         i < parentStack->Height(); i++)
-    {
-        GenTree* node = parentStack->Index(i);
-        switch (node->OperGet())
-        {
-            case GT_LIST:
-            case GT_ARGPLACE:
-                break;
-            case GT_NOP:
-                // Currently there's an issue when the rationalizer performs
-                // the fixup of a call argument: the case is when we remove an
-                // inserted NOP as a parent of a call introduced by fgMorph;
-                // when then the rationalizer removes it, the tree stack in the
-                // walk is not consistent with the node it was just deleted, so the
-                // solution is just to go 1 level deeper.
-                // TODO-Cleanup: This has to be fixed in a proper way: make the rationalizer
-                // correctly modify the evaluation stack when removing treenodes.
-                if (node->gtOp.gtOp1->gtOper == GT_CALL)
-                {
-                    return node->gtOp.gtOp1;
-                }
-                break;
-            case GT_CALL:
-                return node;
-            default:
-                return nullptr;
-        }
-    }
-    return nullptr;
-}
-
 // return op that is the store equivalent of the given load opcode
 genTreeOps storeForm(genTreeOps loadForm)
 {
@@ -109,54 +71,6 @@ void copyFlags(GenTree* dst, GenTree* src, unsigned mask)
     dst->gtFlags |= (src->gtFlags & mask);
 }
 
-// call args have other pointers to them which must be fixed up if
-// they are replaced
-void Compiler::fgFixupIfCallArg(ArrayStack<GenTree*>* parentStack, GenTree* oldChild, GenTree* newChild)
-{
-    GenTree* parentCall = isNodeCallArg(parentStack);
-    if (!parentCall)
-    {
-        return;
-    }
-
-    // we have replaced an arg, so update pointers in argtable
-    fgFixupArgTabEntryPtr(parentCall, oldChild, newChild);
-}
-
-//------------------------------------------------------------------------
-// fgFixupArgTabEntryPtr: Fixup the fgArgTabEntryPtr of parentCall after
-//                        replacing oldArg with newArg
-//
-// Arguments:
-//    parentCall - a pointer to the parent call node
-//    oldArg     - the original argument node
-//    newArg     - the replacement argument node
-//
-
-void Compiler::fgFixupArgTabEntryPtr(GenTreePtr parentCall, GenTreePtr oldArg, GenTreePtr newArg)
-{
-    assert(parentCall != nullptr);
-    assert(oldArg != nullptr);
-    assert(newArg != nullptr);
-
-    JITDUMP("parent call was :\n");
-    DISPNODE(parentCall);
-
-    JITDUMP("old child was :\n");
-    DISPNODE(oldArg);
-
-    if (oldArg->gtFlags & GTF_LATE_ARG)
-    {
-        newArg->gtFlags |= GTF_LATE_ARG;
-    }
-    else
-    {
-        fgArgTabEntryPtr fp = Compiler::gtArgEntryByNode(parentCall, oldArg);
-        assert(fp->node == oldArg);
-        fp->node = newArg;
-    }
-}
-
 // Rewrite a SIMD indirection as GT_IND(GT_LEA(obj.op1)), or as a simple
 // lclVar if possible.
 //
@@ -248,7 +162,16 @@ void Rationalizer::RewriteNodeAsCall(GenTree**             use,
 #endif
 
     // Replace "tree" with "call"
-    *use = call;
+    if (data->parentStack->Height() > 1)
+    {
+        data->parentStack->Index(1)->ReplaceOperand(use, call);
+    }
+    else
+    {
+        // If there's no parent, the tree being replaced is the root of the
+        // statement (and no special handling is necessary).
+        *use = call;
+    }
 
     // Rebuild the evaluation order.
     comp->gtSetStmtInfo(root);
@@ -278,8 +201,6 @@ void Rationalizer::RewriteNodeAsCall(GenTree**             use,
         treeNextNode->gtPrev = treeLastNode;
     }
 
-    comp->fgFixupIfCallArg(data->parentStack, tree, call);
-
     // Propagate flags of "call" to its parents.
     // 0 is current node, so start at 1
     for (int i = 1; i < data->parentStack->Height(); i++)
@@ -945,7 +866,7 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, ArrayStack<G
         BlockRange().Remove(node);
     }
 
-    assert(isLateArg == ((node->gtFlags & GTF_LATE_ARG) != 0));
+    assert(isLateArg == ((use.Def()->gtFlags & GTF_LATE_ARG) != 0));
 
     return Compiler::WALK_CONTINUE;
 }