From e39f7c49112c31032445ebfbfc3cefd10532a03e Mon Sep 17 00:00:00 2001 From: Pat Gavlin Date: Tue, 13 Sep 2016 13:42:14 -0700 Subject: [PATCH] Refactor call arg table updates. 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 | 7 -- src/coreclr/src/jit/gentree.cpp | 130 ++++++++++++++++++++++++------------ src/coreclr/src/jit/gentree.h | 4 ++ src/coreclr/src/jit/lir.cpp | 7 +- src/coreclr/src/jit/lower.cpp | 6 +- src/coreclr/src/jit/rationalize.cpp | 101 +++------------------------- 6 files changed, 107 insertions(+), 148 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 50cc3f9..a3a20a2 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -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* 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); diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 8df171f..3bea668 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -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); } diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 53aca89..5716cae 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -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) { } diff --git a/src/coreclr/src/jit/lir.cpp b/src/coreclr/src/jit/lir.cpp index ed39dce..cd7856f 100644 --- a/src/coreclr/src/jit/lir.cpp +++ b/src/coreclr/src/jit/lir.cpp @@ -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); } } diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index e059407..61bc6c9 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -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; } diff --git a/src/coreclr/src/jit/rationalize.cpp b/src/coreclr/src/jit/rationalize.cpp index 4a204c2..081d642 100644 --- a/src/coreclr/src/jit/rationalize.cpp +++ b/src/coreclr/src/jit/rationalize.cpp @@ -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* 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* 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, ArrayStackgtFlags & GTF_LATE_ARG) != 0)); + assert(isLateArg == ((use.Def()->gtFlags & GTF_LATE_ARG) != 0)); return Compiler::WALK_CONTINUE; } -- 2.7.4