From 47310fb1f21b215d9b190600779c794650d1625d Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 5 Dec 2021 17:14:46 +0300 Subject: [PATCH] Implement "fgSetTreeSeq" via "GenTreeVisitor" Lets one more custom traversal go. Also results in some ~0.17% fewer instructions executed according to PIN. --- src/coreclr/jit/compiler.h | 9 +- src/coreclr/jit/flowgraph.cpp | 389 ++++++---------------------------- src/coreclr/jit/lir.cpp | 2 +- 3 files changed, 64 insertions(+), 336 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 2c6eb8c47d193..b2f5a3dabab74 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5501,14 +5501,7 @@ class Compiler #endif //-------- Determine the order in which the trees will be evaluated ------- - - unsigned fgTreeSeqNum; - GenTree* fgTreeSeqLst; - GenTree* fgTreeSeqBeg; - - GenTree* fgSetTreeSeq(GenTree* tree, GenTree* prev = nullptr, bool isLIR = false); - void fgSetTreeSeqHelper(GenTree* tree, bool isLIR); - void fgSetTreeSeqFinish(GenTree* tree, bool isLIR); + GenTree* fgSetTreeSeq(GenTree* tree, bool isLIR = false); void fgSetStmtSeq(Statement* stmt); void fgSetBlockOrder(BasicBlock* block); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 761912d6c2b69..a2d5cb585214f 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -3843,284 +3843,87 @@ BasicBlock* Compiler::fgRngChkTarget(BasicBlock* block, SpecialCodeKind kind) return fgAddCodeRef(block, bbThrowIndex(block), kind); } -// Sequences the tree. -// prevTree is what gtPrev of the first node in execution order gets set to. -// Returns the first node (execution order) in the sequenced tree. -GenTree* Compiler::fgSetTreeSeq(GenTree* tree, GenTree* prevTree, bool isLIR) -{ - GenTree list; - - if (prevTree == nullptr) - { - prevTree = &list; - } - fgTreeSeqLst = prevTree; - fgTreeSeqNum = 0; - fgTreeSeqBeg = nullptr; - fgSetTreeSeqHelper(tree, isLIR); - - GenTree* result = prevTree->gtNext; - if (prevTree == &list) - { - list.gtNext->gtPrev = nullptr; - } - - return result; -} - -/***************************************************************************** - * - * Assigns sequence numbers to the given tree and its sub-operands, and - * threads all the nodes together via the 'gtNext' and 'gtPrev' fields. - * Uses 'global' - fgTreeSeqLst - */ - -void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR) +//------------------------------------------------------------------------ +// fgSetTreeSeq: Sequence the tree, setting the "gtPrev" and "gtNext" links. +// +// Also sets the sequence numbers for dumps. The last and first node of +// the resulting "range" will have their "gtNext" and "gtPrev" links set +// to "nullptr". +// +// Arguments: +// tree - the tree to sequence +// isLIR - whether the sequencing is being done for LIR. If so, +// ARGPLACE nodes will not be threaded into the linear +// order, and the GTF_REVERSE_OPS flag will be cleared +// on all the nodes. +// +// Return Value: +// The first node to execute in the sequenced tree. +// +GenTree* Compiler::fgSetTreeSeq(GenTree* tree, bool isLIR) { - // TODO-List-Cleanup: measure what using GenTreeVisitor here brings. - genTreeOps oper; - unsigned kind; - - noway_assert(tree); - assert(!IsUninitialized(tree)); - - /* Figure out what kind of a node we have */ - - oper = tree->OperGet(); - kind = tree->OperKind(); - - /* Is this a leaf/constant node? */ - - if (kind & GTK_LEAF) - { - fgSetTreeSeqFinish(tree, isLIR); - return; - } - - /* Is it a 'simple' unary/binary operator? */ - - if (kind & GTK_SMPOP) + class SetTreeSeqVisitor final : public GenTreeVisitor { - GenTree* op1 = tree->AsOp()->gtOp1; - GenTree* op2 = tree->gtGetOp2IfPresent(); + GenTree* m_prevNode; + const bool m_isLIR; - /* Special handling for AddrMode */ - if (tree->OperIsAddrMode()) + public: + enum { - bool reverse = ((tree->gtFlags & GTF_REVERSE_OPS) != 0); - if (reverse) - { - assert(op1 != nullptr && op2 != nullptr); - fgSetTreeSeqHelper(op2, isLIR); - } - if (op1 != nullptr) - { - fgSetTreeSeqHelper(op1, isLIR); - } - if (!reverse && op2 != nullptr) - { - fgSetTreeSeqHelper(op2, isLIR); - } + DoPostOrder = true, + UseExecutionOrder = true + }; - fgSetTreeSeqFinish(tree, isLIR); - return; - } - - /* Check for a nilary operator */ - - if (op1 == nullptr) - { - noway_assert(op2 == nullptr); - fgSetTreeSeqFinish(tree, isLIR); - return; - } - - /* Is this a unary operator? */ - - if (op2 == nullptr) + SetTreeSeqVisitor(Compiler* compiler, GenTree* tree, bool isLIR) + : GenTreeVisitor(compiler), m_prevNode(tree), m_isLIR(isLIR) { - /* Visit the (only) operand and we're done */ - - fgSetTreeSeqHelper(op1, isLIR); - fgSetTreeSeqFinish(tree, isLIR); - return; + INDEBUG(tree->gtSeqNum = 0); } - // By the time execution order is being set, all QMARKs must have been rationalized. - assert(compQmarkRationalized && (oper != GT_QMARK) && (oper != GT_COLON)); - - /* This is a binary operator */ - - if (tree->gtFlags & GTF_REVERSE_OPS) - { - fgSetTreeSeqHelper(op2, isLIR); - fgSetTreeSeqHelper(op1, isLIR); - } - else + fgWalkResult PostOrderVisit(GenTree** use, GenTree* user) { - fgSetTreeSeqHelper(op1, isLIR); - fgSetTreeSeqHelper(op2, isLIR); - } - - fgSetTreeSeqFinish(tree, isLIR); - return; - } - - /* See what kind of a special operator we have here */ + GenTree* node = *use; - switch (oper) - { - case GT_CALL: - - for (CallArg& arg : tree->AsCall()->gtArgs.Args()) - { - fgSetTreeSeqHelper(arg.GetEarlyNode(), isLIR); - } - - for (CallArg& arg : tree->AsCall()->gtArgs.LateArgs()) - { - fgSetTreeSeqHelper(arg.GetLateNode(), isLIR); - } - - if ((tree->AsCall()->gtCallType == CT_INDIRECT) && (tree->AsCall()->gtCallCookie != nullptr)) - { - fgSetTreeSeqHelper(tree->AsCall()->gtCallCookie, isLIR); - } - - if (tree->AsCall()->gtCallType == CT_INDIRECT) + if (m_isLIR) { - fgSetTreeSeqHelper(tree->AsCall()->gtCallAddr, isLIR); - } - - if (tree->AsCall()->gtControlExpr) - { - fgSetTreeSeqHelper(tree->AsCall()->gtControlExpr, isLIR); - } + node->ClearReverseOp(); - break; - -#if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS) -#if defined(FEATURE_SIMD) - case GT_SIMD: -#endif -#if defined(FEATURE_HW_INTRINSICS) - case GT_HWINTRINSIC: -#endif - if (tree->IsReverseOp()) - { - assert(tree->AsMultiOp()->GetOperandCount() == 2); - fgSetTreeSeqHelper(tree->AsMultiOp()->Op(2), isLIR); - fgSetTreeSeqHelper(tree->AsMultiOp()->Op(1), isLIR); - } - else - { - for (GenTree* operand : tree->AsMultiOp()->Operands()) + // ARGPLACE nodes are not threaded into the LIR sequence. + if (node->OperIs(GT_ARGPLACE)) { - fgSetTreeSeqHelper(operand, isLIR); + return fgWalkResult::WALK_CONTINUE; } } - break; -#endif // defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS) - case GT_ARR_ELEM: + node->gtPrev = m_prevNode; + m_prevNode->gtNext = node; - fgSetTreeSeqHelper(tree->AsArrElem()->gtArrObj, isLIR); + INDEBUG(node->gtSeqNum = m_prevNode->gtSeqNum + 1); - unsigned dim; - for (dim = 0; dim < tree->AsArrElem()->gtArrRank; dim++) - { - fgSetTreeSeqHelper(tree->AsArrElem()->gtArrInds[dim], isLIR); - } + m_prevNode = node; - break; - - case GT_ARR_OFFSET: - fgSetTreeSeqHelper(tree->AsArrOffs()->gtOffset, isLIR); - fgSetTreeSeqHelper(tree->AsArrOffs()->gtIndex, isLIR); - fgSetTreeSeqHelper(tree->AsArrOffs()->gtArrObj, isLIR); - break; - - case GT_PHI: - for (GenTreePhi::Use& use : tree->AsPhi()->Uses()) - { - fgSetTreeSeqHelper(use.GetNode(), isLIR); - } - break; - - case GT_FIELD_LIST: - for (GenTreeFieldList::Use& use : tree->AsFieldList()->Uses()) - { - fgSetTreeSeqHelper(use.GetNode(), isLIR); - } - break; - - case GT_CMPXCHG: - // Evaluate the trees left to right - fgSetTreeSeqHelper(tree->AsCmpXchg()->gtOpLocation, isLIR); - fgSetTreeSeqHelper(tree->AsCmpXchg()->gtOpValue, isLIR); - fgSetTreeSeqHelper(tree->AsCmpXchg()->gtOpComparand, isLIR); - break; - - case GT_STORE_DYN_BLK: - fgSetTreeSeqHelper(tree->AsStoreDynBlk()->Addr(), isLIR); - fgSetTreeSeqHelper(tree->AsStoreDynBlk()->Data(), isLIR); - fgSetTreeSeqHelper(tree->AsStoreDynBlk()->gtDynamicSize, isLIR); - break; - - default: -#ifdef DEBUG - gtDispTree(tree); - noway_assert(!"unexpected operator"); -#endif // DEBUG - break; - } - - fgSetTreeSeqFinish(tree, isLIR); -} - -void Compiler::fgSetTreeSeqFinish(GenTree* tree, bool isLIR) -{ - // If we are sequencing for LIR: - // - Clear the reverse ops flag - // - If we are processing a node that does not appear in LIR, do not add it to the list. - if (isLIR) - { - tree->gtFlags &= ~GTF_REVERSE_OPS; + return fgWalkResult::WALK_CONTINUE; + } - if (tree->OperIs(GT_ARGPLACE)) + GenTree* Sequence() { - return; + // We have set "m_prevNode" to "tree" in the constructor - this will give us a + // circular list here ("tree->gtNext == firstNode", "firstNode->gtPrev == tree"). + GenTree* tree = m_prevNode; + WalkTree(&tree, nullptr); + assert(tree == m_prevNode); + + // Extract the first node in the sequence and break the circularity. + GenTree* lastNode = tree; + GenTree* firstNode = lastNode->gtNext; + lastNode->gtNext = nullptr; + firstNode->gtPrev = nullptr; + + return firstNode; } - } - - /* Append to the node list */ - ++fgTreeSeqNum; - -#ifdef DEBUG - tree->gtSeqNum = fgTreeSeqNum; - - if (verbose & 0) - { - printf("SetTreeOrder: "); - printTreeID(fgTreeSeqLst); - printf(" followed by "); - printTreeID(tree); - printf("\n"); - } -#endif // DEBUG - - fgTreeSeqLst->gtNext = tree; - tree->gtNext = nullptr; - tree->gtPrev = fgTreeSeqLst; - fgTreeSeqLst = tree; - - /* Remember the very first node */ + }; - if (!fgTreeSeqBeg) - { - fgTreeSeqBeg = tree; - assert(tree->gtSeqNum == 1); - } + return SetTreeSeqVisitor(this, tree, isLIR).Sequence(); } /***************************************************************************** @@ -4241,81 +4044,13 @@ void Compiler::fgSetBlockOrder() void Compiler::fgSetStmtSeq(Statement* stmt) { - GenTree list; // helper node that we use to start the StmtList - // It's located in front of the first node in the list - - /* Assign numbers and next/prev links for this tree */ - - fgTreeSeqNum = 0; - fgTreeSeqLst = &list; - fgTreeSeqBeg = nullptr; - - fgSetTreeSeqHelper(stmt->GetRootNode(), false); - - /* Record the address of the first node */ - - stmt->SetTreeList(fgTreeSeqBeg); - -#ifdef DEBUG - - if (list.gtNext->gtPrev != &list) - { - printf("&list "); - printTreeID(&list); - printf(" != list.next->prev "); - printTreeID(list.gtNext->gtPrev); - printf("\n"); - goto BAD_LIST; - } - - GenTree* temp; - GenTree* last; - for (temp = list.gtNext, last = &list; temp != nullptr; last = temp, temp = temp->gtNext) - { - if (temp->gtPrev != last) - { - printTreeID(temp); - printf("->gtPrev = "); - printTreeID(temp->gtPrev); - printf(", but last = "); - printTreeID(last); - printf("\n"); - - BAD_LIST:; - - printf("\n"); - gtDispTree(stmt->GetRootNode()); - printf("\n"); - - for (GenTree* bad = &list; bad != nullptr; bad = bad->gtNext) - { - printf(" entry at "); - printTreeID(bad); - printf(" (prev="); - printTreeID(bad->gtPrev); - printf(",next=)"); - printTreeID(bad->gtNext); - printf("\n"); - } - - printf("\n"); - noway_assert(!"Badly linked tree"); - break; - } - } -#endif // DEBUG - - /* Fix the first node's 'prev' link */ - - noway_assert(list.gtNext->gtPrev == &list); - list.gtNext->gtPrev = nullptr; + stmt->SetTreeList(fgSetTreeSeq(stmt->GetRootNode())); #ifdef DEBUG - /* Keep track of the highest # of tree nodes */ - - if (BasicBlock::s_nMaxTrees < fgTreeSeqNum) + // Keep track of the highest # of tree nodes. + if (BasicBlock::s_nMaxTrees < stmt->GetRootNode()->gtSeqNum) { - BasicBlock::s_nMaxTrees = fgTreeSeqNum; + BasicBlock::s_nMaxTrees = stmt->GetRootNode()->gtSeqNum; } #endif // DEBUG } diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index adeeb43f1a3a6..717dd078cc1c0 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -1683,7 +1683,7 @@ LIR::Range LIR::SeqTree(Compiler* compiler, GenTree* tree) // point. compiler->gtSetEvalOrder(tree); - return Range(compiler->fgSetTreeSeq(tree, nullptr, true), tree); + return Range(compiler->fgSetTreeSeq(tree, /* isLIR */ true), tree); } //------------------------------------------------------------------------