From 61808ca1b23d66ef443650d97bd9138f4b806ef0 Mon Sep 17 00:00:00 2001 From: Sergey Date: Wed, 26 May 2021 00:29:04 -0700 Subject: [PATCH 1/3] Delete references to `GT_PHI_ARG/GT_PHI` after rat. --- src/coreclr/jit/decomposelongs.cpp | 3 +-- src/coreclr/jit/lir.cpp | 13 +------------ src/coreclr/jit/lower.cpp | 5 +---- src/coreclr/jit/rationalize.cpp | 28 +++++++++++++++------------- 4 files changed, 18 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/decomposelongs.cpp b/src/coreclr/jit/decomposelongs.cpp index 423319fc5eb5c3..e019e2d0342a42 100644 --- a/src/coreclr/jit/decomposelongs.cpp +++ b/src/coreclr/jit/decomposelongs.cpp @@ -408,8 +408,7 @@ GenTree* DecomposeLongs::DecomposeStoreLclVar(LIR::Use& use) GenTree* tree = use.Def(); GenTree* rhs = tree->gtGetOp1(); - if ((rhs->OperGet() == GT_PHI) || (rhs->OperGet() == GT_CALL) || - ((rhs->OperGet() == GT_MUL_LONG) && (rhs->gtFlags & GTF_MUL_64RSLT) != 0)) + if (rhs->OperIs(GT_CALL) || (rhs->OperIs(GT_MUL_LONG) && (rhs->gtFlags & GTF_MUL_64RSLT) != 0)) { // GT_CALLs are not decomposed, so will not be converted to GT_LONG // GT_STORE_LCL_VAR = GT_CALL are handled in genMultiRegCallStoreToLocal diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index 6bcc97b3ea2ea9..069f4cffc23419 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -1548,8 +1548,7 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const SmallHashTable unusedDefs(compiler->getAllocatorDebugOnly()); - bool pastPhis = false; - GenTree* prev = nullptr; + GenTree* prev = nullptr; for (Iterator node = begin(), end = this->end(); node != end; prev = *node, ++node) { // Verify that the node is allowed in LIR. @@ -1567,16 +1566,6 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const // TODO: validate catch arg stores - // Check that all phi nodes (if any) occur at the start of the range. - if ((node->OperGet() == GT_PHI_ARG) || (node->OperGet() == GT_PHI) || node->IsPhiDefn()) - { - assert(!pastPhis); - } - else - { - pastPhis = true; - } - for (GenTree** useEdge : node->UseEdges()) { GenTree* def = *useEdge; diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index dd430519a3287f..91abea2098ce7d 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2042,9 +2042,6 @@ void Lowering::RehomeArgForFastTailCall(unsigned int lclNum, continue; } - // This should not be a GT_PHI_ARG. - assert(treeNode->OperGet() != GT_PHI_ARG); - GenTreeLclVarCommon* lcl = treeNode->AsLclVarCommon(); if (lcl->GetLclNum() != lclNum) @@ -3118,7 +3115,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore) } CheckMultiRegLclVar(lclStore->AsLclVar(), retTypeDesc); } - if ((lclStore->TypeGet() == TYP_STRUCT) && !srcIsMultiReg && (src->OperGet() != GT_PHI)) + if ((lclStore->TypeGet() == TYP_STRUCT) && !srcIsMultiReg) { bool convertToStoreObj; if (src->OperGet() == GT_CALL) diff --git a/src/coreclr/jit/rationalize.cpp b/src/coreclr/jit/rationalize.cpp index b8dd797f3a0a65..23aa85b5c86acc 100644 --- a/src/coreclr/jit/rationalize.cpp +++ b/src/coreclr/jit/rationalize.cpp @@ -426,7 +426,6 @@ void Rationalizer::RewriteAssignment(LIR::Use& use) { case GT_LCL_VAR: case GT_LCL_FLD: - case GT_PHI_ARG: RewriteAssignmentIntoStoreLclCore(assignment, location, value, locationOp); BlockRange().Remove(location); break; @@ -949,20 +948,23 @@ PhaseStatus Rationalizer::DoPhase() assert(statement->GetRootNode() != nullptr); assert(statement->GetRootNode()->gtNext == nullptr); - BlockRange().InsertAtEnd(LIR::Range(statement->GetTreeList(), statement->GetRootNode())); - - // If this statement has correct offset information, change it into an IL offset - // node and insert it into the LIR. - if (statement->GetILOffsetX() != BAD_IL_OFFSET) + if (!statement->IsPhiDefnStmt()) // Note that we get rid of PHI nodes here. { - assert(!statement->IsPhiDefnStmt()); - GenTreeILOffset* ilOffset = new (comp, GT_IL_OFFSET) - GenTreeILOffset(statement->GetILOffsetX() DEBUGARG(statement->GetLastILOffset())); - BlockRange().InsertBefore(statement->GetTreeList(), ilOffset); - } + BlockRange().InsertAtEnd(LIR::Range(statement->GetTreeList(), statement->GetRootNode())); + + // If this statement has correct offset information, change it into an IL offset + // node and insert it into the LIR. + if (statement->GetILOffsetX() != BAD_IL_OFFSET) + { + assert(!statement->IsPhiDefnStmt()); + GenTreeILOffset* ilOffset = new (comp, GT_IL_OFFSET) + GenTreeILOffset(statement->GetILOffsetX() DEBUGARG(statement->GetLastILOffset())); + BlockRange().InsertBefore(statement->GetTreeList(), ilOffset); + } - m_block = block; - visitor.WalkTree(statement->GetRootNodePointer(), nullptr); + m_block = block; + visitor.WalkTree(statement->GetRootNodePointer(), nullptr); + } } block->bbStmtList = nullptr; From 4aca90f89c482fcadb9f5c4cd8f9099eef76d4f3 Mon Sep 17 00:00:00 2001 From: Sergey Date: Wed, 26 May 2021 01:43:34 -0700 Subject: [PATCH 2/3] Delete references after Rat. --- src/coreclr/jit/block.cpp | 2 +- src/coreclr/jit/codegenlinear.cpp | 4 +- src/coreclr/jit/decomposelongs.cpp | 2 +- src/coreclr/jit/fgopt.cpp | 21 ++------ src/coreclr/jit/lclvars.cpp | 2 +- src/coreclr/jit/lir.cpp | 77 ++---------------------------- src/coreclr/jit/lir.h | 14 ++---- src/coreclr/jit/liveness.cpp | 11 ++--- src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/lsra.cpp | 10 ++-- src/coreclr/jit/lsrabuild.cpp | 2 +- 11 files changed, 29 insertions(+), 118 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 8778245a932ea2..fed7b59d329e75 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -951,7 +951,7 @@ bool BasicBlock::isEmpty() } else { - for (GenTree* node : LIR::AsRange(this).NonPhiNodes()) + for (GenTree* node : LIR::AsRange(this)) { if (node->OperGet() != GT_IL_OFFSET) { diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index d4bbf00562262c..796c128b98eac1 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -403,7 +403,7 @@ void CodeGen::genCodeForBBlist() // Set the use-order numbers for each node. { int useNum = 0; - for (GenTree* node : LIR::AsRange(block).NonPhiNodes()) + for (GenTree* node : LIR::AsRange(block)) { assert((node->gtDebugFlags & GTF_DEBUG_NODE_CG_CONSUMED) == 0); @@ -422,7 +422,7 @@ void CodeGen::genCodeForBBlist() #endif // DEBUG IL_OFFSETX currentILOffset = BAD_IL_OFFSET; - for (GenTree* node : LIR::AsRange(block).NonPhiNodes()) + for (GenTree* node : LIR::AsRange(block)) { // Do we have a new IL offset? if (node->OperGet() == GT_IL_OFFSET) diff --git a/src/coreclr/jit/decomposelongs.cpp b/src/coreclr/jit/decomposelongs.cpp index e019e2d0342a42..f72e0c490f5f17 100644 --- a/src/coreclr/jit/decomposelongs.cpp +++ b/src/coreclr/jit/decomposelongs.cpp @@ -101,7 +101,7 @@ void DecomposeLongs::DecomposeRangeHelper() { assert(m_range != nullptr); - GenTree* node = Range().FirstNonPhiNode(); + GenTree* node = Range().FirstNode(); while (node != nullptr) { node = DecomposeNode(node); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index fb2c991063aad1..985b7e864ce34d 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1659,25 +1659,12 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext) LIR::Range& nextRange = LIR::AsRange(bNext); // Does the next block have any phis? - GenTree* nextFirstNonPhi = nullptr; - LIR::ReadOnlyRange nextPhis = nextRange.PhiNodes(); - if (!nextPhis.IsEmpty()) - { - GenTree* blockLastPhi = blockRange.LastPhiNode(); - nextFirstNonPhi = nextPhis.LastNode()->gtNext; - - LIR::Range phisToMove = nextRange.Remove(std::move(nextPhis)); - blockRange.InsertAfter(blockLastPhi, std::move(phisToMove)); - } - else - { - nextFirstNonPhi = nextRange.FirstNode(); - } + GenTree* nextNode = nextRange.FirstNode(); - // Does the block have any other code? - if (nextFirstNonPhi != nullptr) + // Does the block have any code? + if (nextNode != nullptr) { - LIR::Range nextNodes = nextRange.Remove(nextFirstNonPhi, nextRange.LastNode()); + LIR::Range nextNodes = nextRange.Remove(nextNode, nextRange.LastNode()); blockRange.InsertAtEnd(std::move(nextNodes)); } } diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 85a04da43718a3..16cf0242b04ab3 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -4518,7 +4518,7 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers) assert(isRecompute); const BasicBlock::weight_t weight = block->getBBWeight(this); - for (GenTree* node : LIR::AsRange(block).NonPhiNodes()) + for (GenTree* node : LIR::AsRange(block)) { switch (node->OperGet()) { diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index 069f4cffc23419..b34b35f8a9c367 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -432,57 +432,17 @@ LIR::Range::Range(GenTree* firstNode, GenTree* lastNode) : ReadOnlyRange(firstNo } //------------------------------------------------------------------------ -// LIR::Range::LastPhiNode: Returns the last phi node in the range or -// `nullptr` if no phis exist. +// LIR::Range::FirstNonCatchArgNode: Returns the first node after all catch arg nodes in this range. // -GenTree* LIR::Range::LastPhiNode() const +GenTree* LIR::Range::FirstNonCatchArgNode() const { - GenTree* lastPhiNode = nullptr; for (GenTree* node : *this) { - if (!node->IsPhiNode()) - { - break; - } - - lastPhiNode = node; - } - - return lastPhiNode; -} - -//------------------------------------------------------------------------ -// LIR::Range::FirstNonPhiNode: Returns the first non-phi node in the -// range or `nullptr` if no non-phi nodes -// exist. -// -GenTree* LIR::Range::FirstNonPhiNode() const -{ - for (GenTree* node : *this) - { - if (!node->IsPhiNode()) - { - return node; - } - } - - return nullptr; -} - -//------------------------------------------------------------------------ -// LIR::Range::FirstNonPhiOrCatchArgNode: Returns the first node after all -// phi or catch arg nodes in this -// range. -// -GenTree* LIR::Range::FirstNonPhiOrCatchArgNode() const -{ - for (GenTree* node : NonPhiNodes()) - { - if (node->OperGet() == GT_CATCH_ARG) + if (node->OperIs(GT_CATCH_ARG)) { continue; } - else if ((node->OperGet() == GT_STORE_LCL_VAR) && (node->gtGetOp1()->OperGet() == GT_CATCH_ARG)) + else if ((node->OperIs(GT_STORE_LCL_VAR)) && (node->gtGetOp1()->OperIs(GT_CATCH_ARG))) { continue; } @@ -493,35 +453,6 @@ GenTree* LIR::Range::FirstNonPhiOrCatchArgNode() const return nullptr; } -//------------------------------------------------------------------------ -// LIR::Range::PhiNodes: Returns the range of phi nodes inside this range. -// -LIR::ReadOnlyRange LIR::Range::PhiNodes() const -{ - GenTree* lastPhiNode = LastPhiNode(); - if (lastPhiNode == nullptr) - { - return ReadOnlyRange(); - } - - return ReadOnlyRange(m_firstNode, lastPhiNode); -} - -//------------------------------------------------------------------------ -// LIR::Range::PhiNodes: Returns the range of non-phi nodes inside this -// range. -// -LIR::ReadOnlyRange LIR::Range::NonPhiNodes() const -{ - GenTree* firstNonPhiNode = FirstNonPhiNode(); - if (firstNonPhiNode == nullptr) - { - return ReadOnlyRange(); - } - - return ReadOnlyRange(firstNonPhiNode, m_lastNode); -} - //------------------------------------------------------------------------ // LIR::Range::InsertBefore: Inserts a node before another node in this range. // diff --git a/src/coreclr/jit/lir.h b/src/coreclr/jit/lir.h index e174482be6b36e..a700bb16728558 100644 --- a/src/coreclr/jit/lir.h +++ b/src/coreclr/jit/lir.h @@ -92,15 +92,14 @@ class LIR final // // View the block as a range // LIR::Range& blockRange = LIR::AsRange(block); // - // // Create a range from the first non-phi node in the block to the - // // last node in the block - // LIR::ReadOnlyRange nonPhis = blockRange.NonPhiNodes(); + // // Create a read only range from from it. + // LIR::ReadOnlyRange readRange = blockRange; // // // Remove the last node from the block // blockRange.Remove(blockRange.LastNode()); // // After the removal of the last node in the block, the last node of - // nonPhis is no longer linked to any of the other nodes in nonPhis. Due + // readRange is no longer linked to any of the other nodes in readRange. Due // to issues such as the above, some care must be taken in order to // ensure that ranges are not used once they have been invalidated. // @@ -256,12 +255,7 @@ class LIR final Range(); Range(Range&& other); - GenTree* LastPhiNode() const; - GenTree* FirstNonPhiNode() const; - GenTree* FirstNonPhiOrCatchArgNode() const; - - ReadOnlyRange PhiNodes() const; - ReadOnlyRange NonPhiNodes() const; + GenTree* FirstNonCatchArgNode() const; void InsertBefore(GenTree* insertionPoint, GenTree* node); void InsertAfter(GenTree* insertionPoint, GenTree* node); diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index cae25c9d593530..20366bd8f1b5e2 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -501,7 +501,7 @@ void Compiler::fgPerBlockLocalVarLiveness() compCurBB = block; if (block->IsLIR()) { - for (GenTree* node : LIR::AsRange(block).NonPhiNodes()) + for (GenTree* node : LIR::AsRange(block)) { fgPerNodeLocalVarLiveness(node); } @@ -1873,14 +1873,13 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR noway_assert(VarSetOps::IsSubset(this, keepAliveVars, life)); - LIR::Range& blockRange = LIR::AsRange(block); - GenTree* firstNonPhiNode = blockRange.FirstNonPhiNode(); - if (firstNonPhiNode == nullptr) + LIR::Range& blockRange = LIR::AsRange(block); + GenTree* firstNode = blockRange.FirstNode(); + if (firstNode == nullptr) { return; } - for (GenTree *node = blockRange.LastNode(), *next = nullptr, *end = firstNonPhiNode->gtPrev; node != end; - node = next) + for (GenTree *node = blockRange.LastNode(), *next = nullptr, *end = firstNode->gtPrev; node != end; node = next) { next = node->gtPrev; diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 91abea2098ce7d..05976b1b37ee2d 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4037,7 +4037,7 @@ void Lowering::InsertPInvokeMethodProlog() store->AsOp()->gtOp1 = call; store->gtFlags |= GTF_VAR_DEF; - GenTree* const insertionPoint = firstBlockRange.FirstNonPhiOrCatchArgNode(); + GenTree* const insertionPoint = firstBlockRange.FirstNonCatchArgNode(); comp->fgMorphTree(store); firstBlockRange.InsertBefore(insertionPoint, LIR::SeqTree(comp, store)); diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index d5c4e952067631..30ddcc3af4bea8 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -7803,7 +7803,7 @@ void LinearScan::handleOutgoingCriticalEdges(BasicBlock* block) // so if we have only EH vars, we'll do that instead of splitting the edge. if ((compiler->compHndBBtabCount > 0) && VarSetOps::IsSubset(compiler, edgeResolutionSet, exceptVars)) { - GenTree* insertionPoint = LIR::AsRange(succBlock).FirstNonPhiNode(); + GenTree* insertionPoint = LIR::AsRange(succBlock).FirstNode(); VarSetOps::Iter edgeSetIter(compiler, edgeResolutionSet); unsigned edgeVarIndex = 0; while (edgeSetIter.NextElem(&edgeVarIndex)) @@ -8178,7 +8178,7 @@ void LinearScan::resolveEdge(BasicBlock* fromBlock, GenTree* insertionPoint = nullptr; if (resolveType == ResolveSplit || resolveType == ResolveCritical) { - insertionPoint = LIR::AsRange(block).FirstNonPhiNode(); + insertionPoint = LIR::AsRange(block).FirstNode(); } // If this is an edge between EH regions, we may have "extra" live-out EH vars. @@ -9436,7 +9436,7 @@ void LinearScan::TupleStyleDump(LsraTupleDumpMode mode) splitEdgeInfo.toBBNum); } - for (GenTree* node : LIR::AsRange(block).NonPhiNodes()) + for (GenTree* node : LIR::AsRange(block)) { GenTree* tree = node; @@ -10358,7 +10358,7 @@ void LinearScan::verifyFinalAllocation() bool foundNonResolutionNode = false; LIR::Range& currentBlockRange = LIR::AsRange(currentBlock); - for (GenTree* node : currentBlockRange.NonPhiNodes()) + for (GenTree* node : currentBlockRange) { if (IsResolutionNode(currentBlockRange, node)) { @@ -10666,7 +10666,7 @@ void LinearScan::verifyFinalAllocation() // Verify the moves in this block LIR::Range& currentBlockRange = LIR::AsRange(currentBlock); - for (GenTree* node : currentBlockRange.NonPhiNodes()) + for (GenTree* node : currentBlockRange) { assert(IsResolutionNode(currentBlockRange, node)); if (IsResolutionMove(node)) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 83999c3589b2ac..db02482b6b436a 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -2334,7 +2334,7 @@ void LinearScan::buildIntervals() } LIR::Range& blockRange = LIR::AsRange(block); - for (GenTree* node : blockRange.NonPhiNodes()) + for (GenTree* node : blockRange) { // We increment the location of each tree node by 2 so that the node definition, if any, // is at a new location and doesn't interfere with the uses. From 83ca346c07064a7d3ab92de1c93b2fb80d03163c Mon Sep 17 00:00:00 2001 From: Sergey Date: Wed, 26 May 2021 01:53:32 -0700 Subject: [PATCH 3/3] add check that we don't see them. --- src/coreclr/jit/lower.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 05976b1b37ee2d..242c3717904ae1 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6046,6 +6046,11 @@ void Lowering::CheckNode(Compiler* compiler, GenTree* node) break; } + case GT_PHI: + case GT_PHI_ARG: + assert(!"Should not see phi nodes after rationalize"); + break; + default: break; }