From 51c9148460fb9d708b021d17f0cc92b732ef1e0f Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Wed, 5 Jan 2022 22:56:15 +0300 Subject: [PATCH 1/4] Clean things up a little Delete redundant conditions, use "LclVarDsc*", rename locals for clarity. --- src/coreclr/jit/copyprop.cpp | 81 +++++++++++++++++------------------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 1c80436902f90..be69452398922 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -137,11 +137,8 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc } // If not local nothing to do. - if (!tree->IsLocal()) - { - return; - } - if (tree->OperGet() == GT_PHI_ARG || tree->OperGet() == GT_LCL_FLD) + // TODO-CQ: allow LCL_FLDs too. + if (!tree->OperIs(GT_LCL_VAR)) { return; } @@ -151,6 +148,7 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc { return; } + const unsigned lclNum = optIsSsaLocal(tree); // Skip non-SSA variables. @@ -165,17 +163,20 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc { unsigned newLclNum = iter.Get(); - GenTree* op = iter.GetValue()->Top(); - // Nothing to do if same. if (lclNum == newLclNum) { continue; } + LclVarDsc* varDsc = lvaGetDesc(lclNum); + LclVarDsc* newLclVarDsc = lvaGetDesc(newLclNum); + GenTree* newLclDefNode = iter.GetValue()->Top(); // Note that this "def" node can actually be a use (for + // parameters and other use-before-def locals). + // Skip variables with assignments embedded in the statement (i.e., with a comma). Because we // are not currently updating their SSA names as live in the copy-prop pass of the stmt. - if (VarSetOps::IsMember(this, optCopyPropKillSet, lvaTable[newLclNum].lvVarIndex)) + if (VarSetOps::IsMember(this, optCopyPropKillSet, newLclVarDsc->lvVarIndex)) { continue; } @@ -186,34 +187,39 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc // However, in addition, it may not be profitable to propagate a 'doNotEnregister' lclVar to an // existing use of an enregisterable lclVar. - if (lvaTable[lclNum].lvDoNotEnregister != lvaTable[newLclNum].lvDoNotEnregister) + if (varDsc->lvDoNotEnregister != newLclVarDsc->lvDoNotEnregister) { continue; } - if (op->gtFlags & GTF_VAR_CAST) + if (newLclDefNode->gtFlags & GTF_VAR_CAST) { continue; } - if (gsShadowVarInfo != nullptr && lvaTable[newLclNum].lvIsParam && - gsShadowVarInfo[newLclNum].shadowCopy == lclNum) + + if ((gsShadowVarInfo != nullptr) && newLclVarDsc->lvIsParam && + (gsShadowVarInfo[newLclNum].shadowCopy == lclNum)) { continue; } - ValueNum opVN = GetUseAsgDefVNOrTreeVN(op); - if (opVN == ValueNumStore::NoVN) + + ValueNum newLclDefVN = GetUseAsgDefVNOrTreeVN(newLclDefNode); + if (newLclDefVN == ValueNumStore::NoVN) { continue; } - if (op->TypeGet() != tree->TypeGet()) + + if (newLclDefNode->TypeGet() != tree->TypeGet()) { continue; } - if (opVN != tree->gtVNPair.GetConservative()) + + if (newLclDefVN != tree->gtVNPair.GetConservative()) { continue; } - if (optCopyProp_LclVarScore(lvaGetDesc(lclNum), lvaGetDesc(newLclNum), true) <= 0) + + if (optCopyProp_LclVarScore(varDsc, newLclVarDsc, true) <= 0) { continue; } @@ -230,34 +236,25 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc // node x2 = phi(x0, x1) which can then be used to substitute 'c' with. But because of pruning // there would be no such phi node. To solve this we'll check if 'x' is live, before replacing // 'c' with 'x.' - if (!lvaTable[newLclNum].lvVerTypeInfo.IsThisPtr()) - { - if (lvaTable[newLclNum].IsAddressExposed()) - { - continue; - } - // We compute liveness only on tracked variables. So skip untracked locals. - if (!lvaTable[newLclNum].lvTracked) - { - continue; - } + // We compute liveness only on tracked variables. And all SSA locals are tracked. + assert(lvaGetDesc(newLclNum)->lvTracked); - // Because of this dependence on live variable analysis, CopyProp phase is immediately - // after Liveness, SSA and VN. - if (!VarSetOps::IsMember(this, compCurLife, lvaTable[newLclNum].lvVarIndex)) - { - continue; - } + // Because of this dependence on live variable analysis, CopyProp phase is immediately + // after Liveness, SSA and VN. + if ((newLclNum != info.compThisArg) && !VarSetOps::IsMember(this, compCurLife, newLclVarDsc->lvVarIndex)) + { + continue; } + unsigned newSsaNum = SsaConfig::RESERVED_SSA_NUM; - if (op->gtFlags & GTF_VAR_DEF) + if (newLclDefNode->gtFlags & GTF_VAR_DEF) { - newSsaNum = GetSsaNumForLocalVarDef(op); + newSsaNum = GetSsaNumForLocalVarDef(newLclDefNode); } else // parameters, this pointer etc. { - newSsaNum = op->AsLclVarCommon()->GetSsaNum(); + newSsaNum = newLclDefNode->AsLclVarCommon()->GetSsaNum(); } if (newSsaNum == SsaConfig::RESERVED_SSA_NUM) @@ -271,25 +268,25 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc JITDUMP("VN based copy assertion for "); printTreeID(tree); printf(" V%02d " FMT_VN " by ", lclNum, tree->GetVN(VNK_Conservative)); - printTreeID(op); - printf(" V%02d " FMT_VN ".\n", newLclNum, op->GetVN(VNK_Conservative)); - gtDispTree(tree, nullptr, nullptr, true); + printTreeID(newLclDefNode); + printf(" V%02d " FMT_VN ".\n", newLclNum, newLclDefNode->GetVN(VNK_Conservative)); + DISPNODE(tree); } #endif tree->AsLclVarCommon()->SetLclNum(newLclNum); tree->AsLclVarCommon()->SetSsaNum(newSsaNum); gtUpdateSideEffects(stmt, tree); + #ifdef DEBUG if (verbose) { printf("copy propagated to:\n"); - gtDispTree(tree, nullptr, nullptr, true); + DISPNODE(tree); } #endif break; } - return; } //------------------------------------------------------------------------------ From 01a6e5e2b9f016ee6395ef11b8d46d078ba79481 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 6 Jan 2022 00:24:59 +0300 Subject: [PATCH 2/4] Delete a redundant condition For actual def nodes, GTF_VAR_CAST will never be set, it is only set in "optNarrowTree" for uses. For "def nodes" that are actually uses (parameters), the VNs will never match anyway. --- src/coreclr/jit/copyprop.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index be69452398922..62503978eff86 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -192,11 +192,6 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc continue; } - if (newLclDefNode->gtFlags & GTF_VAR_CAST) - { - continue; - } - if ((gsShadowVarInfo != nullptr) && newLclVarDsc->lvIsParam && (gsShadowVarInfo[newLclNum].shadowCopy == lclNum)) { From 4011712e10e97a9b8aa48639d71339a3eacb34ef Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 6 Jan 2022 16:10:04 +0300 Subject: [PATCH 3/4] Handle embedded assignments in copy propagation Previously, as the comments in copy propagation tell us, it did not handle "intervening", or not-top-level definitions of locals, instead opting to maintain a dedicated kill set of them. This is obviously a CQ problem, but also a TP one, as it meant there had to be a second pass over the statement's IR, where the definitions would be pushed on the stack. This change does away with that, instead pushing new definitions as they are encountered in execution order, and simultaneously propagating on uses. Notably, this means the code now needs to look at the real definition nodes, i. e. ASGs, not the LHS locals, as those are encountered first in canonical execution order, i. e. for a tree like: ``` ASG LCL_VAR V00 "def" ADD LCL_VAR V00 LCL_VAR V00 ``` Were we to use the "def" as the definition point, we would wrongly push it as the definition on the stack, even as the assignments itself hasn't happened yet at that point. There are nice diffs with this change, all resulting from unblocked propagations, and mostly coming from setup arguments under calls. --- src/coreclr/jit/compiler.h | 5 +- src/coreclr/jit/copyprop.cpp | 148 +++++++++++++---------------------- 2 files changed, 56 insertions(+), 97 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 5a16ac8a58bf8..85027f8426695 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7370,11 +7370,8 @@ class Compiler typedef ArrayStack GenTreePtrStack; typedef JitHashTable, GenTreePtrStack*> LclNumToGenTreePtrStack; - // Kill set to track variables with intervening definitions. - VARSET_TP optCopyPropKillSet; - // Copy propagation functions. - void optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, LclNumToGenTreePtrStack* curSsaName); + void optCopyProp(Statement* stmt, GenTreeLclVarCommon* tree, unsigned lclNum, LclNumToGenTreePtrStack* curSsaName); void optBlockCopyPropPopStacks(BasicBlock* block, LclNumToGenTreePtrStack* curSsaName); void optBlockCopyProp(BasicBlock* block, LclNumToGenTreePtrStack* curSsaName); unsigned optIsSsaLocal(GenTree* tree); diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 62503978eff86..fe19dd351d040 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -32,17 +32,16 @@ void Compiler::optBlockCopyPropPopStacks(BasicBlock* block, LclNumToGenTreePtrSt { for (GenTree* const tree : stmt->TreeList()) { - if (!tree->IsLocal()) - { - continue; - } - const unsigned lclNum = optIsSsaLocal(tree); - if (lclNum == BAD_VAR_NUM) - { - continue; - } - if (tree->gtFlags & GTF_VAR_DEF) + GenTreeLclVarCommon* lclDefNode = nullptr; + if (tree->OperIs(GT_ASG) && tree->DefinesLocal(this, &lclDefNode)) { + const unsigned lclNum = optIsSsaLocal(lclDefNode); + + if (lclNum == BAD_VAR_NUM) + { + continue; + } + GenTreePtrStack* stack = nullptr; curSsaName->Lookup(lclNum, &stack); stack->Pop(); @@ -123,40 +122,17 @@ int Compiler::optCopyProp_LclVarScore(LclVarDsc* lclVarDsc, LclVarDsc* copyVarDs // definitions share the same value number. If so, then we can make the replacement. // // Arguments: -// block - Block the tree belongs to // stmt - Statement the tree belongs to -// tree - The tree to perform copy propagation on +// tree - The local tree to perform copy propagation on +// lclNum - The local number of said tree // curSsaName - The map from lclNum to its recently live definitions as a stack -void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, LclNumToGenTreePtrStack* curSsaName) +void Compiler::optCopyProp(Statement* stmt, + GenTreeLclVarCommon* tree, + unsigned lclNum, + LclNumToGenTreePtrStack* curSsaName) { - // TODO-Review: EH successor/predecessor iteration seems broken. - if (block->bbCatchTyp == BBCT_FINALLY || block->bbCatchTyp == BBCT_FAULT) - { - return; - } - - // If not local nothing to do. - // TODO-CQ: allow LCL_FLDs too. - if (!tree->OperIs(GT_LCL_VAR)) - { - return; - } - - // Propagate only on uses. - if (tree->gtFlags & GTF_VAR_DEF) - { - return; - } - - const unsigned lclNum = optIsSsaLocal(tree); - - // Skip non-SSA variables. - if (lclNum == BAD_VAR_NUM) - { - return; - } - + assert((lclNum != BAD_VAR_NUM) && (optIsSsaLocal(tree) == lclNum) && ((tree->gtFlags & GTF_VAR_DEF) == 0)); assert(tree->gtVNPair.GetConservative() != ValueNumStore::NoVN); for (LclNumToGenTreePtrStack::KeyIterator iter = curSsaName->Begin(); !iter.Equal(curSsaName->End()); ++iter) @@ -174,19 +150,11 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc GenTree* newLclDefNode = iter.GetValue()->Top(); // Note that this "def" node can actually be a use (for // parameters and other use-before-def locals). - // Skip variables with assignments embedded in the statement (i.e., with a comma). Because we - // are not currently updating their SSA names as live in the copy-prop pass of the stmt. - if (VarSetOps::IsMember(this, optCopyPropKillSet, newLclVarDsc->lvVarIndex)) - { - continue; - } - // Do not copy propagate if the old and new lclVar have different 'doNotEnregister' settings. // This is primarily to avoid copy propagating to IND(ADDR(LCL_VAR)) where the replacement lclVar // is not marked 'lvDoNotEnregister'. // However, in addition, it may not be profitable to propagate a 'doNotEnregister' lclVar to an // existing use of an enregisterable lclVar. - if (varDsc->lvDoNotEnregister != newLclVarDsc->lvDoNotEnregister) { continue; @@ -347,68 +315,63 @@ void Compiler::optBlockCopyProp(BasicBlock* block, LclNumToGenTreePtrStack* curS VarSetOps::Assign(this, compCurLife, block->bbLiveIn); for (Statement* const stmt : block->Statements()) { - VarSetOps::ClearD(this, optCopyPropKillSet); - // Walk the tree to find if any local variable can be replaced with current live definitions. + // Simultaneously, push live definitions on the stack - that logic must be in sync with the + // SSA renaming process. for (GenTree* const tree : stmt->TreeList()) { treeLifeUpdater.UpdateLife(tree); - optCopyProp(block, stmt, tree, curSsaName); - - // TODO-Review: Merge this loop with the following loop to correctly update the - // live SSA num while also propagating copies. - // - // 1. This loop performs copy prop with currently live (on-top-of-stack) SSA num. - // 2. The subsequent loop maintains a stack for each lclNum with - // currently active SSA numbers when definitions are encountered. - // - // If there is an embedded definition using a "comma" in a stmt, then the currently - // live SSA number will get updated only in the next loop (2). However, this new - // definition is now supposed to be live (on tos). If we did not update the stacks - // using (2), copy prop (1) will use a SSA num defined outside the stmt ignoring the - // embedded update. Killing the variable is a simplification to produce 0 ASM diffs - // for an update release. - // - const unsigned lclNum = optIsSsaLocal(tree); - if ((lclNum != BAD_VAR_NUM) && (tree->gtFlags & GTF_VAR_DEF)) + GenTreeLclVarCommon* lclDefNode = nullptr; + if (tree->OperIs(GT_ASG) && tree->DefinesLocal(this, &lclDefNode)) { - VarSetOps::AddElemD(this, optCopyPropKillSet, lvaTable[lclNum].lvVarIndex); - } - } + const unsigned lclNum = optIsSsaLocal(lclDefNode); - // This logic must be in sync with SSA renaming process. - for (GenTree* const tree : stmt->TreeList()) - { - const unsigned lclNum = optIsSsaLocal(tree); - if (lclNum == BAD_VAR_NUM) - { - continue; - } + if (lclNum == BAD_VAR_NUM) + { + continue; + } - // As we encounter a definition add it to the stack as a live definition. - if (tree->gtFlags & GTF_VAR_DEF) - { GenTreePtrStack* stack; if (!curSsaName->Lookup(lclNum, &stack)) { stack = new (curSsaName->GetAllocator()) GenTreePtrStack(curSsaName->GetAllocator()); } - stack->Push(tree); + stack->Push(lclDefNode); curSsaName->Set(lclNum, stack, LclNumToGenTreePtrStack::Overwrite); } - // If we encounter first use of a param or this pointer add it as a live definition. - // Since they are always live, do it only once. - else if ((tree->gtOper == GT_LCL_VAR) && !(tree->gtFlags & GTF_VAR_USEASG) && - (lvaTable[lclNum].lvIsParam || lvaTable[lclNum].lvVerTypeInfo.IsThisPtr())) + // TODO-CQ: propagate on LCL_FLDs too. + else if (tree->OperIs(GT_LCL_VAR) && ((tree->gtFlags & GTF_VAR_DEF) == 0)) { - GenTreePtrStack* stack; - if (!curSsaName->Lookup(lclNum, &stack)) + const unsigned lclNum = optIsSsaLocal(tree); + + if (lclNum == BAD_VAR_NUM) { - stack = new (curSsaName->GetAllocator()) GenTreePtrStack(curSsaName->GetAllocator()); - stack->Push(tree); - curSsaName->Set(lclNum, stack); + continue; } + + // If we encounter first use of a param or this pointer add it as a live definition. + // Since they are always live, we'll do it only once. + if (lvaGetDesc(lclNum)->lvIsParam || (lclNum == info.compThisArg)) + { + GenTreePtrStack* stack; + if (!curSsaName->Lookup(lclNum, &stack)) + { + assert(tree->AsLclVarCommon()->GetSsaNum() == SsaConfig::FIRST_SSA_NUM); + + stack = new (curSsaName->GetAllocator()) GenTreePtrStack(curSsaName->GetAllocator()); + stack->Push(tree); + curSsaName->Set(lclNum, stack); + } + } + + // TODO-Review: EH successor/predecessor iteration seems broken. + if ((block->bbCatchTyp == BBCT_FINALLY) || (block->bbCatchTyp == BBCT_FAULT)) + { + continue; + } + + optCopyProp(stmt, tree->AsLclVarCommon(), lclNum, curSsaName); } } } @@ -454,7 +417,6 @@ void Compiler::optVnCopyProp() } VarSetOps::AssignNoCopy(this, compCurLife, VarSetOps::MakeEmpty(this)); - VarSetOps::AssignNoCopy(this, optCopyPropKillSet, VarSetOps::MakeEmpty(this)); class CopyPropDomTreeVisitor : public DomTreeVisitor { From db9a3f97c81afb324525c16e275da01df3256913 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 6 Jan 2022 16:31:08 +0300 Subject: [PATCH 4/4] Simplify optIsSsaLocal --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/copyprop.cpp | 16 +++++----------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 85027f8426695..b799d5bd0ebf6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7374,7 +7374,7 @@ class Compiler void optCopyProp(Statement* stmt, GenTreeLclVarCommon* tree, unsigned lclNum, LclNumToGenTreePtrStack* curSsaName); void optBlockCopyPropPopStacks(BasicBlock* block, LclNumToGenTreePtrStack* curSsaName); void optBlockCopyProp(BasicBlock* block, LclNumToGenTreePtrStack* curSsaName); - unsigned optIsSsaLocal(GenTree* tree); + unsigned optIsSsaLocal(GenTreeLclVarCommon* lclNode); int optCopyProp_LclVarScore(LclVarDsc* lclVarDsc, LclVarDsc* copyVarDsc, bool preferOp2); void optVnCopyProp(); INDEBUG(void optDumpCopyPropStack(LclNumToGenTreePtrStack* curSsaName)); diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index fe19dd351d040..52c765a12dc41 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -256,23 +256,17 @@ void Compiler::optCopyProp(Statement* stmt, // optIsSsaLocal : helper to check if the tree is a local that participates in SSA numbering. // // Arguments: -// tree - The tree to perform the check on; +// lclNode - The local tree to perform the check on; // // Returns: // - lclNum if the local is participating in SSA; // - fieldLclNum if the parent local can be replaced by its only field; // - BAD_VAR_NUM otherwise. // -unsigned Compiler::optIsSsaLocal(GenTree* tree) +unsigned Compiler::optIsSsaLocal(GenTreeLclVarCommon* lclNode) { - if (!tree->IsLocal()) - { - return BAD_VAR_NUM; - } - - GenTreeLclVarCommon* lclNode = tree->AsLclVarCommon(); - unsigned lclNum = lclNode->GetLclNum(); - LclVarDsc* varDsc = lvaGetDesc(lclNum); + unsigned lclNum = lclNode->GetLclNum(); + LclVarDsc* varDsc = lvaGetDesc(lclNum); if (!lvaInSsa(lclNum) && varDsc->CanBeReplacedWithItsField(this)) { @@ -343,7 +337,7 @@ void Compiler::optBlockCopyProp(BasicBlock* block, LclNumToGenTreePtrStack* curS // TODO-CQ: propagate on LCL_FLDs too. else if (tree->OperIs(GT_LCL_VAR) && ((tree->gtFlags & GTF_VAR_DEF) == 0)) { - const unsigned lclNum = optIsSsaLocal(tree); + const unsigned lclNum = optIsSsaLocal(tree->AsLclVarCommon()); if (lclNum == BAD_VAR_NUM) {