From 4f19db652a1729ab8c6619d5c8f5d50f18357f0b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 10 Jul 2024 17:56:23 -0700 Subject: [PATCH 1/6] restructure VN block visit order --- src/coreclr/jit/compiler.h | 3 + src/coreclr/jit/valuenum.cpp | 103 ++++++++++++++++++++++++++--------- 2 files changed, 79 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index bb6adaaa5472a..2a14465ddf831 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5701,6 +5701,9 @@ class Compiler // Utility functions for fgValueNumber. + // Value number a block or blocks in a loop + void fgValueNumberBlocks(BasicBlock* block, BlockSet& visitedBlocks); + // Perform value-numbering for the trees in "blk". void fgValueNumberBlock(BasicBlock* blk); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 624b0a5396796..a7489dee3ed95 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -10375,48 +10375,97 @@ PhaseStatus Compiler::fgValueNumber() // SSA has already computed a post-order taking EH successors into account. // Visiting that in reverse will ensure we visit a block's predecessors // before itself whenever possible. + // + EnsureBasicBlockEpoch(); + BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this)); BasicBlock** postOrder = m_dfsTree->GetPostOrder(); unsigned postOrderCount = m_dfsTree->GetPostOrderCount(); for (unsigned i = postOrderCount; i != 0; i--) { - BasicBlock* block = postOrder[i - 1]; - JITDUMP("Visiting " FMT_BB "\n", block->bbNum); + BasicBlock* const block = postOrder[i - 1]; + fgValueNumberBlocks(block, visitedBlocks); + } - if (block != fgFirstBB) - { - bool anyPredReachable = false; - for (FlowEdge* pred = BlockPredsWithEH(block); pred != nullptr; pred = pred->getNextPredEdge()) - { - BasicBlock* predBlock = pred->getSourceBlock(); - if (!vs.IsReachableThroughPred(block, predBlock)) - { - JITDUMP(" Unreachable through pred " FMT_BB "\n", predBlock->bbNum); - continue; - } +#ifdef DEBUG + JitTestCheckVN(); + fgDebugCheckExceptionSets(); +#endif // DEBUG - JITDUMP(" Reachable through pred " FMT_BB "\n", predBlock->bbNum); - anyPredReachable = true; - break; - } + fgVNPassesCompleted++; + + return PhaseStatus::MODIFIED_EVERYTHING; +} - if (!anyPredReachable) +//------------------------------------------------------------------------ +// fgValueNumberBlocks: Run value numbering for a block or blocks in a loop +// +// Arguments: +// block -- block to value number (may already have been numbered) +// visitedBlocks -- blocks that have already had VNs assigned +// +// Notes: +// +// Within the overall reverse post order, we want to ensure we visit all blocks in +// a loop before any possible loop block successors. So if we visit a loop +// header, we switch to visiting the loop blocks in RPO, and once we finish +// that visitation, we try and refine loop header PHIs. +// +void Compiler::fgValueNumberBlocks(BasicBlock* block, BlockSet& visitedBlocks) +{ + // Because we're not following the strict RPO, we may have already visisted this block. + // + if (BlockSetOps::IsMember(this, visitedBlocks, block->bbNum)) + { + return; + } + + JITDUMP("Visiting " FMT_BB "\n", block->bbNum); + + if (block != fgFirstBB) + { + bool anyPredReachable = false; + for (FlowEdge* pred = BlockPredsWithEH(block); pred != nullptr; pred = pred->getNextPredEdge()) + { + BasicBlock* predBlock = pred->getSourceBlock(); + if (!vnState->IsReachableThroughPred(block, predBlock)) { - JITDUMP(" " FMT_BB " was proven unreachable\n", block->bbNum); - vs.SetUnreachable(block); + JITDUMP(" Unreachable through pred " FMT_BB "\n", predBlock->bbNum); + continue; } + + JITDUMP(" Reachable through pred " FMT_BB "\n", predBlock->bbNum); + anyPredReachable = true; + break; } - fgValueNumberBlock(block); + if (!anyPredReachable) + { + JITDUMP(" " FMT_BB " was proven unreachable\n", block->bbNum); + vnState->SetUnreachable(block); + } } -#ifdef DEBUG - JitTestCheckVN(); - fgDebugCheckExceptionSets(); -#endif // DEBUG + fgValueNumberBlock(block); - fgVNPassesCompleted++; + // Mark block as visited + // + BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); - return PhaseStatus::MODIFIED_EVERYTHING; + // Is block the head of a loop? + // + FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(block); + if ((loop != nullptr) && (block == loop->GetHeader())) + { + // Yes. Visit all other loop blocks using the within-loop RPO. + // + loop->VisitLoopBlocksReversePostOrder([&](BasicBlock* block) { + fgValueNumberBlocks(block, visitedBlocks); + return BasicBlockVisit::Continue; + }); + + // TODO: Postprocess to refine phis of block. + // + } } void Compiler::fgValueNumberBlock(BasicBlock* blk) From 12ef1b05c4a2615a83bd6b46f8438d0fe10c6001 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 10 Jul 2024 20:04:49 -0700 Subject: [PATCH 2/6] renumber phis at loop end --- src/coreclr/jit/compiler.h | 3 + src/coreclr/jit/copyprop.cpp | 14 +-- src/coreclr/jit/valuenum.cpp | 181 +++++++++++++++++++---------------- 3 files changed, 108 insertions(+), 90 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 2a14465ddf831..073a227e71cb2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5707,6 +5707,9 @@ class Compiler // Perform value-numbering for the trees in "blk". void fgValueNumberBlock(BasicBlock* blk); + // Value number a phi definition + void fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* block); + // Requires that "entryBlock" is the header block of "loop" and that "loop" is the // innermost loop of which "entryBlock" is the entry. Returns the value number that should be // assumed for the memoryKind at the start "entryBlk". diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 11bce9e358e3d..22c375aa79e14 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -166,6 +166,8 @@ bool Compiler::optCopyProp( ValueNum lclDefVN = varDsc->GetPerSsaData(tree->GetSsaNum())->m_vnPair.GetConservative(); assert(lclDefVN != ValueNumStore::NoVN); + JITDUMP("Considering [%06u] with VN " FMT_VN "\n", dspTreeID(tree), lclDefVN); + for (LclNumToLiveDefsMap::Node* const iter : LclNumToLiveDefsMap::KeyValueIteration(curSsaName)) { unsigned newLclNum = iter->GetKey(); @@ -182,6 +184,7 @@ bool Compiler::optCopyProp( // Likewise, nothing to do if the most recent def is not available. if (newLclSsaDef == nullptr) { + JITDUMP("... missing def for [%06u]\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -190,6 +193,7 @@ bool Compiler::optCopyProp( if (newLclDefVN != lclDefVN) { + JITDUMP("... [%06u] VN mismatch " FMT_VN "\n", dspTreeID(newLclDef.GetDefNode()), newLclDefVN); continue; } @@ -198,11 +202,13 @@ bool Compiler::optCopyProp( LclVarDsc* const newLclVarDsc = lvaGetDesc(newLclNum); if (varDsc->lvDoNotEnregister != newLclVarDsc->lvDoNotEnregister) { + JITDUMP("... [%06u] DNER mismatch\n", dspTreeID(newLclDef.GetDefNode())); continue; } if (optCopyProp_LclVarScore(varDsc, newLclVarDsc, true) <= 0) { + JITDUMP("... [%06u] score not good\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -227,6 +233,7 @@ bool Compiler::optCopyProp( // after Liveness, SSA and VN. if ((newLclNum != info.compThisArg) && !VarSetOps::IsMember(this, compCurLife, newLclVarDsc->lvVarIndex)) { + JITDUMP("... [%06u] liveness not good\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -239,6 +246,7 @@ bool Compiler::optCopyProp( var_types oldLclType = tree->OperIs(GT_LCL_VAR) ? tree->TypeGet() : varDsc->TypeGet(); if (newLclType != oldLclType) { + JITDUMP("... [%06u] type mismatch\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -334,12 +342,6 @@ void Compiler::optCopyPropPushDef(GenTree* defNode, GenTreeLclVarCommon* lclNode else if (lclNode->HasSsaName()) { unsigned ssaNum = lclNode->GetSsaNum(); - if ((defNode != nullptr) && defNode->IsPhiDefn()) - { - // TODO-CQ: design better heuristics for propagation and remove this. - ssaNum = SsaConfig::RESERVED_SSA_NUM; - } - pushDef(lclNum, ssaNum); } } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index a7489dee3ed95..45c43e75ac8a2 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -10463,7 +10463,15 @@ void Compiler::fgValueNumberBlocks(BasicBlock* block, BlockSet& visitedBlocks) return BasicBlockVisit::Continue; }); - // TODO: Postprocess to refine phis of block. + // Re-value number all the PhiDefs in block + // + for (Statement* stmt = block->firstStmt(); (stmt != nullptr) && stmt->IsPhiDefnStmt(); stmt = stmt->GetNextStmt()) + { + GenTreeLclVar* const newSsaDef = stmt->GetRootNode()->AsLclVar(); + fgValueNumberPhiDef(newSsaDef, block); + } + + // TODO: Propagate those new VNs to their uses // } } @@ -10478,89 +10486,7 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) for (; (stmt != nullptr) && stmt->IsPhiDefnStmt(); stmt = stmt->GetNextStmt()) { GenTreeLclVar* newSsaDef = stmt->GetRootNode()->AsLclVar(); - GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); - ValueNumPair phiVNP; - ValueNumPair sameVNP; - - for (GenTreePhi::Use& use : phiNode->Uses()) - { - GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); - if (!vnState->IsReachableThroughPred(blk, phiArg->gtPredBB)) - { - JITDUMP(" Phi arg [%06u] is unnecessary; path through pred " FMT_BB " cannot be taken\n", - dspTreeID(phiArg), phiArg->gtPredBB->bbNum); - - if ((use.GetNext() != nullptr) || (phiVNP.GetLiberal() != ValueNumStore::NoVN)) - { - continue; - } - - assert(!vnState->IsReachable(blk)); - JITDUMP(" ..but no other path can, so we are using it anyway\n"); - } - - ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); - ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; - - phiArg->gtVNPair = phiArgVNP; - - if (phiVNP.GetLiberal() == ValueNumStore::NoVN) - { - // This is the first PHI argument - phiVNP = ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN); - sameVNP = phiArgVNP; - } - else - { - phiVNP = vnStore->VNPairForFuncNoFolding(newSsaDef->TypeGet(), VNF_Phi, - ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN), phiVNP); - - if ((sameVNP.GetLiberal() != phiArgVNP.GetLiberal()) || - (sameVNP.GetConservative() != phiArgVNP.GetConservative())) - { - // If this argument's VNs are different from "same" then change "same" to NoVN. - // Note that this means that if any argument's VN is NoVN then the final result - // will also be NoVN, which is what we want. - sameVNP.SetBoth(ValueNumStore::NoVN); - } - } - } - - // We should have visited at least one phi arg in the loop above - assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); - assert(phiVNP.GetConservative() != ValueNumStore::NoVN); - - ValueNumPair newSsaDefVNP; - - if (sameVNP.BothDefined()) - { - // If all the args of the phi had the same value(s, liberal and conservative), then there wasn't really - // a reason to have the phi -- just pass on that value. - newSsaDefVNP = sameVNP; - } - else - { - // They were not the same; we need to create a phi definition. - ValueNum lclNumVN = ValueNum(newSsaDef->GetLclNum()); - ValueNum ssaNumVN = ValueNum(newSsaDef->GetSsaNum()); - - newSsaDefVNP = vnStore->VNPairForFunc(newSsaDef->TypeGet(), VNF_PhiDef, ValueNumPair(lclNumVN, lclNumVN), - ValueNumPair(ssaNumVN, ssaNumVN), phiVNP); - } - - LclSsaVarDsc* newSsaDefDsc = lvaGetDesc(newSsaDef)->GetPerSsaData(newSsaDef->GetSsaNum()); - newSsaDefDsc->m_vnPair = newSsaDefVNP; -#ifdef DEBUG - if (verbose) - { - printf("SSA PHI definition: set VN of local %d/%d to ", newSsaDef->GetLclNum(), newSsaDef->GetSsaNum()); - vnpPrint(newSsaDefVNP, 1); - printf(" %s.\n", sameVNP.BothDefined() ? "(all same)" : ""); - } -#endif // DEBUG - - newSsaDef->gtVNPair = vnStore->VNPForVoid(); - phiNode->gtVNPair = newSsaDefVNP; + fgValueNumberPhiDef(newSsaDef, blk); } // Now do the same for each MemoryKind. @@ -10707,6 +10633,93 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) compCurBB = nullptr; } +void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) +{ + GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); + ValueNumPair phiVNP; + ValueNumPair sameVNP; + + for (GenTreePhi::Use& use : phiNode->Uses()) + { + GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); + if (!vnState->IsReachableThroughPred(blk, phiArg->gtPredBB)) + { + JITDUMP(" Phi arg [%06u] is unnecessary; path through pred " FMT_BB " cannot be taken\n", + dspTreeID(phiArg), phiArg->gtPredBB->bbNum); + + if ((use.GetNext() != nullptr) || (phiVNP.GetLiberal() != ValueNumStore::NoVN)) + { + continue; + } + + // assert(!vnState->IsReachable(blk)); + JITDUMP(" ..but no other path can, so we are using it anyway\n"); + } + + ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); + ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; + + phiArg->gtVNPair = phiArgVNP; + + if (phiVNP.GetLiberal() == ValueNumStore::NoVN) + { + // This is the first PHI argument + phiVNP = ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN); + sameVNP = phiArgVNP; + } + else + { + phiVNP = vnStore->VNPairForFuncNoFolding(newSsaDef->TypeGet(), VNF_Phi, + ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN), phiVNP); + + if ((sameVNP.GetLiberal() != phiArgVNP.GetLiberal()) || + (sameVNP.GetConservative() != phiArgVNP.GetConservative())) + { + // If this argument's VNs are different from "same" then change "same" to NoVN. + // Note that this means that if any argument's VN is NoVN then the final result + // will also be NoVN, which is what we want. + sameVNP.SetBoth(ValueNumStore::NoVN); + } + } + } + + // We should have visited at least one phi arg in the loop above + assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); + assert(phiVNP.GetConservative() != ValueNumStore::NoVN); + + ValueNumPair newSsaDefVNP; + + if (sameVNP.BothDefined()) + { + // If all the args of the phi had the same value(s, liberal and conservative), then there wasn't really + // a reason to have the phi -- just pass on that value. + newSsaDefVNP = sameVNP; + } + else + { + // They were not the same; we need to create a phi definition. + ValueNum lclNumVN = ValueNum(newSsaDef->GetLclNum()); + ValueNum ssaNumVN = ValueNum(newSsaDef->GetSsaNum()); + + newSsaDefVNP = vnStore->VNPairForFunc(newSsaDef->TypeGet(), VNF_PhiDef, ValueNumPair(lclNumVN, lclNumVN), + ValueNumPair(ssaNumVN, ssaNumVN), phiVNP); + } + + LclSsaVarDsc* newSsaDefDsc = lvaGetDesc(newSsaDef)->GetPerSsaData(newSsaDef->GetSsaNum()); + newSsaDefDsc->m_vnPair = newSsaDefVNP; +#ifdef DEBUG + if (verbose) + { + printf("SSA PHI definition: set VN of local %d/%d to ", newSsaDef->GetLclNum(), newSsaDef->GetSsaNum()); + vnpPrint(newSsaDefVNP, 1); + printf(" %s.\n", sameVNP.BothDefined() ? "(all same)" : ""); + } +#endif // DEBUG + + newSsaDef->gtVNPair = vnStore->VNPForVoid(); + phiNode->gtVNPair = newSsaDefVNP; +} + ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind memoryKind, BasicBlock* entryBlock, FlowGraphNaturalLoop* loop) From c65176e77b98c9b0aac3f6c42c0f9608b3603331 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 11 Jul 2024 10:38:04 -0700 Subject: [PATCH 3/6] check for phi equivalence in copy prop --- src/coreclr/jit/copyprop.cpp | 36 +++++--- src/coreclr/jit/valuenum.cpp | 173 +++++++++++++++++++++++++++++++---- src/coreclr/jit/valuenum.h | 7 ++ 3 files changed, 185 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 22c375aa79e14..a9316cee69670 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -161,12 +161,24 @@ bool Compiler::optCopyProp( assert((tree->gtFlags & GTF_VAR_DEF) == 0); assert(tree->GetLclNum() == lclNum); - bool madeChanges = false; - LclVarDsc* varDsc = lvaGetDesc(lclNum); - ValueNum lclDefVN = varDsc->GetPerSsaData(tree->GetSsaNum())->m_vnPair.GetConservative(); + bool madeChanges = false; + LclVarDsc* const varDsc = lvaGetDesc(lclNum); + LclSsaVarDsc* const varSsaDsc = varDsc->GetPerSsaData(tree->GetSsaNum()); + GenTree* const varDefTree = varSsaDsc->GetDefNode(); + BasicBlock* const varDefBlock = varSsaDsc->GetBlock(); + ValueNum const lclDefVN = varSsaDsc->m_vnPair.GetConservative(); assert(lclDefVN != ValueNumStore::NoVN); - JITDUMP("Considering [%06u] with VN " FMT_VN "\n", dspTreeID(tree), lclDefVN); + // See if this local is a candidate for phi dev equivalence checks + // + bool const varDefTreeIsPhiDef = (varDefTree != nullptr) && varDefTree->IsPhiDefn(); + bool varDefTreeIsPhiDefAtCycleEntry = false; + + if (varDefTreeIsPhiDef) + { + FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(varDefBlock); + varDefTreeIsPhiDefAtCycleEntry = (loop != nullptr) && (loop->GetHeader() == varDefBlock); + } for (LclNumToLiveDefsMap::Node* const iter : LclNumToLiveDefsMap::KeyValueIteration(curSsaName)) { @@ -184,7 +196,6 @@ bool Compiler::optCopyProp( // Likewise, nothing to do if the most recent def is not available. if (newLclSsaDef == nullptr) { - JITDUMP("... missing def for [%06u]\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -193,8 +204,15 @@ bool Compiler::optCopyProp( if (newLclDefVN != lclDefVN) { - JITDUMP("... [%06u] VN mismatch " FMT_VN "\n", dspTreeID(newLclDef.GetDefNode()), newLclDefVN); - continue; + bool arePhiDefsEquivalent = + varDefTreeIsPhiDefAtCycleEntry && vnStore->AreVNsEquivalent(lclDefVN, newLclDefVN); + if (!arePhiDefsEquivalent) + { + continue; + } + + JITDUMP("orig [%06] copy [%06u] VNs proved equivalent\n", dspTreeID(tree), + dspTreeID(newLclDef.GetDefNode())); } // It may not be profitable to propagate a 'doNotEnregister' lclVar to an existing use of an @@ -202,13 +220,11 @@ bool Compiler::optCopyProp( LclVarDsc* const newLclVarDsc = lvaGetDesc(newLclNum); if (varDsc->lvDoNotEnregister != newLclVarDsc->lvDoNotEnregister) { - JITDUMP("... [%06u] DNER mismatch\n", dspTreeID(newLclDef.GetDefNode())); continue; } if (optCopyProp_LclVarScore(varDsc, newLclVarDsc, true) <= 0) { - JITDUMP("... [%06u] score not good\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -233,7 +249,6 @@ bool Compiler::optCopyProp( // after Liveness, SSA and VN. if ((newLclNum != info.compThisArg) && !VarSetOps::IsMember(this, compCurLife, newLclVarDsc->lvVarIndex)) { - JITDUMP("... [%06u] liveness not good\n", dspTreeID(newLclDef.GetDefNode())); continue; } @@ -246,7 +261,6 @@ bool Compiler::optCopyProp( var_types oldLclType = tree->OperIs(GT_LCL_VAR) ? tree->TypeGet() : varDsc->TypeGet(); if (newLclType != oldLclType) { - JITDUMP("... [%06u] type mismatch\n", dspTreeID(newLclDef.GetDefNode())); continue; } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 45c43e75ac8a2..5b44e13f65e39 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6770,6 +6770,138 @@ const char* ValueNumStore::VNRelationString(VN_RELATION_KIND vrk) } #endif +bool ValueNumStore::IsVNPhiDef(ValueNum vn) +{ + VNFuncApp funcAttr; + if (!GetVNFunc(vn, &funcAttr)) + { + return false; + } + + return funcAttr.m_func == VNF_PhiDef; +} +//------------------------------------------------------------------------ +// AreVNsEquivalent: returns true iff VNs represent the same value +// +// Arguments: +// vn1 - first value number to consider +// vn2 - second value number ot consider +// +// Notes: +// Normally if vn1 != vn2 then we cannot say if the two values +// are equivalent or different. PhiDef VNs don't represent the +// phi def values in this way, and can be proven equivalent even +// for different value numbers. +// +bool ValueNumStore::AreVNsEquivalent(ValueNum vn1, ValueNum vn2) +{ + if (vn1 == vn2) + { + return true; + } + + VNFuncApp funcAttr1; + if (!GetVNFunc(vn1, &funcAttr1)) + { + return false; + } + + if (funcAttr1.m_func != VNF_PhiDef) + { + return false; + } + + VNFuncApp funcAttr2; + if (!GetVNFunc(vn2, &funcAttr2)) + { + return false; + } + + if (funcAttr2.m_func != VNF_PhiDef) + { + return false; + } + + // We have two PhiDefs. They may be equivalent, if + // they come from Phis in the same block. + // + const unsigned lclNum1 = unsigned(funcAttr1.m_args[0]); + const unsigned ssaDefNum1 = unsigned(funcAttr1.m_args[1]); + + LclVarDsc* const varDsc1 = m_pComp->lvaGetDesc(lclNum1); + LclSsaVarDsc* const varSsaDsc1 = varDsc1->GetPerSsaData(ssaDefNum1); + GenTree* const varDefTree1 = varSsaDsc1->GetDefNode(); + BasicBlock* const varDefBlock1 = varSsaDsc1->GetBlock(); + + const unsigned lclNum2 = unsigned(funcAttr2.m_args[0]); + const unsigned ssaDefNum2 = unsigned(funcAttr2.m_args[1]); + + LclVarDsc* const varDsc2 = m_pComp->lvaGetDesc(lclNum2); + LclSsaVarDsc* const varSsaDsc2 = varDsc2->GetPerSsaData(ssaDefNum2); + GenTree* const varDefTree2 = varSsaDsc2->GetDefNode(); + BasicBlock* const varDefBlock2 = varSsaDsc2->GetBlock(); + + if (varDefBlock1 != varDefBlock2) + { + return false; + } + + if ((varDefTree1 == nullptr) || (varDefTree2 == nullptr)) + { + return false; + } + + // PhiDefs are from same block. Walk the phi args + // + GenTreePhi* const treePhi1 = varDefTree1->AsLclVar()->Data()->AsPhi(); + GenTreePhi* const treePhi2 = varDefTree2->AsLclVar()->Data()->AsPhi(); + GenTreePhi::UseIterator treeIter1 = treePhi1->Uses().begin(); + GenTreePhi::UseIterator treeEnd1 = treePhi1->Uses().end(); + GenTreePhi::UseIterator treeIter2 = treePhi2->Uses().begin(); + GenTreePhi::UseIterator treeEnd2 = treePhi2->Uses().end(); + + bool phiArgsAreEquivalent = true; + + for (; (treeIter1 != treeEnd1) && (treeIter2 != treeEnd2); ++treeIter1, ++treeIter2) + { + GenTreePhiArg* const treePhiArg1 = treeIter1->GetNode()->AsPhiArg(); + GenTreePhiArg* const treePhiArg2 = treeIter2->GetNode()->AsPhiArg(); + + assert(treePhiArg1->gtPredBB == treePhiArg2->gtPredBB); + + ValueNum treePhiArgVN1 = treePhiArg1->gtVNPair.GetConservative(); + ValueNum treePhiArgVN2 = treePhiArg2->gtVNPair.GetConservative(); + + // If the PhiArg VNs differ, the phis are not equivalent. + // (Note we don't recurse into AreVNsEquivalent as we can't + // handle possible cycles in the SSA graph). + // + if (treePhiArgVN1 != treePhiArgVN2) + { + phiArgsAreEquivalent = false; + break; + } + + // If we failed to find meaningful VNs, the phis are not equivalent + // + if (treePhiArgVN1 == ValueNumStore::NoVN) + { + phiArgsAreEquivalent = false; + break; + } + } + + // If we didn't verify all phi args we have failed to prove equivalence + // + if (phiArgsAreEquivalent) + { + phiArgsAreEquivalent &= (treeIter1 == treeEnd1); + phiArgsAreEquivalent &= (treeIter2 == treeEnd2); + } + + return phiArgsAreEquivalent; +} + bool ValueNumStore::IsVNRelop(ValueNum vn) { VNFuncApp funcAttr; @@ -10465,7 +10597,8 @@ void Compiler::fgValueNumberBlocks(BasicBlock* block, BlockSet& visitedBlocks) // Re-value number all the PhiDefs in block // - for (Statement* stmt = block->firstStmt(); (stmt != nullptr) && stmt->IsPhiDefnStmt(); stmt = stmt->GetNextStmt()) + for (Statement* stmt = block->firstStmt(); (stmt != nullptr) && stmt->IsPhiDefnStmt(); + stmt = stmt->GetNextStmt()) { GenTreeLclVar* const newSsaDef = stmt->GetRootNode()->AsLclVar(); fgValueNumberPhiDef(newSsaDef, block); @@ -10635,32 +10768,32 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) { - GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); - ValueNumPair phiVNP; - ValueNumPair sameVNP; - + GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); + ValueNumPair phiVNP; + ValueNumPair sameVNP; + for (GenTreePhi::Use& use : phiNode->Uses()) { GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); if (!vnState->IsReachableThroughPred(blk, phiArg->gtPredBB)) { JITDUMP(" Phi arg [%06u] is unnecessary; path through pred " FMT_BB " cannot be taken\n", - dspTreeID(phiArg), phiArg->gtPredBB->bbNum); - + dspTreeID(phiArg), phiArg->gtPredBB->bbNum); + if ((use.GetNext() != nullptr) || (phiVNP.GetLiberal() != ValueNumStore::NoVN)) { continue; } - + // assert(!vnState->IsReachable(blk)); JITDUMP(" ..but no other path can, so we are using it anyway\n"); } - + ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; - + phiArg->gtVNPair = phiArgVNP; - + if (phiVNP.GetLiberal() == ValueNumStore::NoVN) { // This is the first PHI argument @@ -10670,8 +10803,8 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) else { phiVNP = vnStore->VNPairForFuncNoFolding(newSsaDef->TypeGet(), VNF_Phi, - ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN), phiVNP); - + ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN), phiVNP); + if ((sameVNP.GetLiberal() != phiArgVNP.GetLiberal()) || (sameVNP.GetConservative() != phiArgVNP.GetConservative())) { @@ -10682,13 +10815,13 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) } } } - + // We should have visited at least one phi arg in the loop above assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); assert(phiVNP.GetConservative() != ValueNumStore::NoVN); - + ValueNumPair newSsaDefVNP; - + if (sameVNP.BothDefined()) { // If all the args of the phi had the same value(s, liberal and conservative), then there wasn't really @@ -10700,11 +10833,11 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) // They were not the same; we need to create a phi definition. ValueNum lclNumVN = ValueNum(newSsaDef->GetLclNum()); ValueNum ssaNumVN = ValueNum(newSsaDef->GetSsaNum()); - + newSsaDefVNP = vnStore->VNPairForFunc(newSsaDef->TypeGet(), VNF_PhiDef, ValueNumPair(lclNumVN, lclNumVN), - ValueNumPair(ssaNumVN, ssaNumVN), phiVNP); + ValueNumPair(ssaNumVN, ssaNumVN), phiVNP); } - + LclSsaVarDsc* newSsaDefDsc = lvaGetDesc(newSsaDef)->GetPerSsaData(newSsaDef->GetSsaNum()); newSsaDefDsc->m_vnPair = newSsaDefVNP; #ifdef DEBUG @@ -10715,7 +10848,7 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) printf(" %s.\n", sameVNP.BothDefined() ? "(all same)" : ""); } #endif // DEBUG - + newSsaDef->gtVNPair = vnStore->VNPForVoid(); phiNode->gtVNPair = newSsaDefVNP; } diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index ce33be31e19b0..0138e6e5aecf7 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1066,6 +1066,13 @@ class ValueNumStore // Returns true iff the VN represents a relop bool IsVNRelop(ValueNum vn); + // Returns true iff the VN is a phi definition + bool IsVNPhiDef(ValueNum vn); + + // Returns true if the two VNs represent the same value + // despite being different VNs. Useful for phi def VNs. + bool AreVNsEquivalent(ValueNum vn1, ValueNum vn2); + enum class VN_RELATION_KIND { VRK_Inferred, // (x ? y) From 920848c6e897bf9870d62c059accf88822bd5d62 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 11 Jul 2024 13:14:58 -0700 Subject: [PATCH 4/6] tolerate updated VN in AP --- src/coreclr/jit/assertionprop.cpp | 21 +++++++++++++--- src/coreclr/jit/compiler.h | 9 ++++++- src/coreclr/jit/valuenum.cpp | 42 ++++++++++++++++++++++++++----- 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 3a3139db4a157..2d75b405f65ba 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1466,9 +1466,24 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, assertion.op1.vn = optConservativeNormalVN(op1); assertion.op1.lcl.ssaNum = op1->AsLclVarCommon()->GetSsaNum(); - assert((assertion.op1.lcl.ssaNum == SsaConfig::RESERVED_SSA_NUM) || - (assertion.op1.vn == vnStore->VNConservativeNormalValue( - lvaGetDesc(lclNum)->GetPerSsaData(assertion.op1.lcl.ssaNum)->m_vnPair))); +#ifdef DEBUG + + // If we're ssa based, check that the VN is reasonable. + // + if (assertion.op1.lcl.ssaNum != SsaConfig::RESERVED_SSA_NUM) + { + LclSsaVarDsc* const ssaDsc = lvaGetDesc(lclNum)->GetPerSsaData(assertion.op1.lcl.ssaNum); + + bool doesVNMatch = (assertion.op1.vn == vnStore->VNConservativeNormalValue(ssaDsc->m_vnPair)); + + if (!doesVNMatch && ssaDsc->m_updated) + { + doesVNMatch = (assertion.op1.vn == vnStore->VNConservativeNormalValue(ssaDsc->m_origVNPair)); + } + + assert(doesVNMatch); + } +#endif ssize_t cnsValue = 0; GenTreeFlags iconFlags = GTF_EMPTY; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 073a227e71cb2..07e0cd6966d2e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -311,6 +311,13 @@ class LclSsaVarDsc } ValueNumPair m_vnPair; + +#ifdef DEBUG + // True if this ssa def VN was updated + bool m_updated = false; + // Originally assigned VN + ValueNumPair m_origVNPair; +#endif }; // This class stores information associated with a memory SSA definition. @@ -5708,7 +5715,7 @@ class Compiler void fgValueNumberBlock(BasicBlock* blk); // Value number a phi definition - void fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* block); + void fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* block, bool isUpdate = false); // Requires that "entryBlock" is the header block of "loop" and that "loop" is the // innermost loop of which "entryBlock" is the entry. Returns the value number that should be diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 5b44e13f65e39..924c598a84fbc 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -10601,10 +10601,10 @@ void Compiler::fgValueNumberBlocks(BasicBlock* block, BlockSet& visitedBlocks) stmt = stmt->GetNextStmt()) { GenTreeLclVar* const newSsaDef = stmt->GetRootNode()->AsLclVar(); - fgValueNumberPhiDef(newSsaDef, block); + fgValueNumberPhiDef(newSsaDef, block, /* isUpdate */ true); } - // TODO: Propagate those new VNs to their uses + // TODO: Propagate those new VNs to their uses? // } } @@ -10766,7 +10766,16 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) compCurBB = nullptr; } -void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) +//------------------------------------------------------------------------ +// fgValueNumberRegisterConstFieldSeq: If a VN'd integer constant has a +// field sequence we want to keep track of, then register it in the side table. +// +// Arguments: +// newSsaDef - lcl var node representing a phi definition +// blk - block with the phi +// isUpdate - true if the phi has already been numbered and this is a renumbering +// +void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bool isUpdate) { GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); ValueNumPair phiVNP; @@ -10785,13 +10794,22 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) continue; } - // assert(!vnState->IsReachable(blk)); + assert(!vnState->IsReachable(blk) || isUpdate); JITDUMP(" ..but no other path can, so we are using it anyway\n"); } ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; + if (isUpdate && (phiArgVNP != phiArg->gtVNPair)) + { + JITDUMP("Updating phi arg [%06u] VN from ", dspTreeID(phiArg)); + JITDUMPEXEC(vnpPrint(phiArg->gtVNPair, 0)); + JITDUMP(" to "); + JITDUMPEXEC(vnpPrint(phiArgVNP, 0)); + JITDUMP("\n"); + } + phiArg->gtVNPair = phiArgVNP; if (phiVNP.GetLiberal() == ValueNumStore::NoVN) @@ -10839,7 +10857,18 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) } LclSsaVarDsc* newSsaDefDsc = lvaGetDesc(newSsaDef)->GetPerSsaData(newSsaDef->GetSsaNum()); - newSsaDefDsc->m_vnPair = newSsaDefVNP; + +#ifdef DEBUG + if (isUpdate) + { + assert(!newSsaDefDsc->m_updated); + newSsaDefDsc->m_origVNPair = newSsaDefDsc->m_vnPair; + newSsaDefDsc->m_updated = true; + } +#endif + + newSsaDefDsc->m_vnPair = newSsaDefVNP; + #ifdef DEBUG if (verbose) { @@ -10850,7 +10879,8 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk) #endif // DEBUG newSsaDef->gtVNPair = vnStore->VNPForVoid(); - phiNode->gtVNPair = newSsaDefVNP; + + phiNode->gtVNPair = newSsaDefVNP; } ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind memoryKind, From ccdca2a74c065f2ae0f86fecdfc783cac788af5c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 15 Jul 2024 12:02:12 -0700 Subject: [PATCH 5/6] Use BBF_NO_CSE_IN more widely --- src/coreclr/jit/redundantbranchopts.cpp | 59 ++++++++++++++++--------- 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 9e44637f99190..b12ab304c0857 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1628,6 +1628,31 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) // JITDUMP("Optimizing via jump threading\n"); + bool setNoCseIn = false; + + // If this is a phi-based threading, and the block we're bypassing has + // a memory phi, mark the successor blocks with BBF_NO_CSE_IN so we can + // block unsound CSE propagation. + // + if (jti.m_isPhiBased) + { + for (MemoryKind memoryKind : allMemoryKinds()) + { + if ((memoryKind == ByrefExposed) && byrefStatesMatchGcHeapStates) + { + continue; + } + + if (jti.m_block->bbMemorySsaPhiFunc[memoryKind] != nullptr) + { + JITDUMP(FMT_BB " has %s memory phi; will be marking blocks with BBF_NO_CSE_IN\n", jti.m_block->bbNum, + memoryKindNames[memoryKind]); + setNoCseIn = true; + break; + } + } + } + // Now reroute the flow from the predecessors. // If this pred is in the set that will reuse block, do nothing. // Else revise pred to branch directly to the appropriate successor of block. @@ -1638,6 +1663,11 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) // if (BlockSetOps::IsMember(this, jti.m_ambiguousPreds, predBlock->bbNum)) { + if (setNoCseIn && !jti.m_block->HasFlag(BBF_NO_CSE_IN)) + { + JITDUMP(FMT_BB " => BBF_NO_CSE_IN\n", jti.m_block->bbNum); + jti.m_block->SetFlags(BBF_NO_CSE_IN); + } continue; } @@ -1652,6 +1682,12 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) predBlock->bbNum, jti.m_block->bbNum, predBlock->bbNum, jti.m_trueTarget->bbNum); fgReplaceJumpTarget(predBlock, jti.m_block, jti.m_trueTarget); + + if (setNoCseIn && !jti.m_trueTarget->HasFlag(BBF_NO_CSE_IN)) + { + JITDUMP(FMT_BB " => BBF_NO_CSE_IN\n", jti.m_trueTarget->bbNum); + jti.m_trueTarget->SetFlags(BBF_NO_CSE_IN); + } } else { @@ -1660,28 +1696,11 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) predBlock->bbNum, jti.m_block->bbNum, predBlock->bbNum, jti.m_falseTarget->bbNum); fgReplaceJumpTarget(predBlock, jti.m_block, jti.m_falseTarget); - } - } - // If this is a phi-based threading, and the block we're bypassing has - // a memory phi, mark the block with BBF_NO_CSE_IN so we can block CSE propagation - // into the block. - // - if (jti.m_isPhiBased) - { - for (MemoryKind memoryKind : allMemoryKinds()) - { - if ((memoryKind == ByrefExposed) && byrefStatesMatchGcHeapStates) - { - continue; - } - - if (jti.m_block->bbMemorySsaPhiFunc[memoryKind] != nullptr) + if (setNoCseIn && !jti.m_falseTarget->HasFlag(BBF_NO_CSE_IN)) { - JITDUMP(FMT_BB " has %s memory phi; marking as BBF_NO_CSE_IN\n", jti.m_block->bbNum, - memoryKindNames[memoryKind]); - jti.m_block->SetFlags(BBF_NO_CSE_IN); - break; + JITDUMP(FMT_BB " => BBF_NO_CSE_IN\n", jti.m_falseTarget->bbNum); + jti.m_falseTarget->SetFlags(BBF_NO_CSE_IN); } } } From 6a21ba339ce15857ccf6d4ea30ddb5e1e2fff051 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 16 Jul 2024 13:34:08 -0700 Subject: [PATCH 6/6] update vn during copy prop, fix dump issue --- src/coreclr/jit/copyprop.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index a9316cee69670..f3c858bc9bcaa 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -211,7 +211,7 @@ bool Compiler::optCopyProp( continue; } - JITDUMP("orig [%06] copy [%06u] VNs proved equivalent\n", dspTreeID(tree), + JITDUMP("orig [%06u] copy [%06u] VNs proved equivalent\n", dspTreeID(tree), dspTreeID(newLclDef.GetDefNode())); } @@ -281,6 +281,24 @@ bool Compiler::optCopyProp( tree->AsLclVarCommon()->SetLclNum(newLclNum); tree->AsLclVarCommon()->SetSsaNum(newSsaNum); + + // Update VN to match, and propagate up through any enclosing commas. + // (we could in principle try updating through other parents, but + // we lack VN's context for memory, so can't get them all). + // + if (newLclDefVN != lclDefVN) + { + tree->SetVNs(newLclSsaDef->m_vnPair); + GenTree* parent = tree->gtGetParent(nullptr); + + while ((parent != nullptr) && parent->OperIs(GT_COMMA)) + { + JITDUMP(" Updating COMMA parent VN [%06u]\n", dspTreeID(parent)); + ValueNumPair op1Xvnp = vnStore->VNPExceptionSet(parent->AsOp()->gtOp1->gtVNPair); + parent->SetVNs(vnStore->VNPWithExc(parent->AsOp()->gtOp2->gtVNPair, op1Xvnp)); + parent = tree->gtGetParent(nullptr); + } + } gtUpdateSideEffects(stmt, tree); newLclSsaDef->AddUse(block);