From f455188a11c440eb14575b01eaf9a3f62d853806 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 17 Jul 2024 11:26:12 -0700 Subject: [PATCH] Value num refine phis (#104752) VN doesn't always give PHI defs the best possible values (in particular if there are backedge PHI args). Revise VN to run intg a "loop-respecting" RPO where we don't visit any loop successors until all loop blocks have been visited. Once the loop is done, update the header PHI VNs since all PHI arg VNs are now known. Then look for equivalent PHI defs in copy prop and enable copy prop when two locals have the same values at the head of a loop. Addresses the regression case noted in #95645 (comment) where cross-block morph's copy prop plus loop bottom testing has created some unnecessary loop-carried values. Closes #95645. --- src/coreclr/jit/assertionprop.cpp | 21 +- src/coreclr/jit/compiler.h | 13 + src/coreclr/jit/copyprop.cpp | 54 ++- src/coreclr/jit/redundantbranchopts.cpp | 59 ++-- src/coreclr/jit/valuenum.cpp | 431 ++++++++++++++++++------ src/coreclr/jit/valuenum.h | 7 + 6 files changed, 449 insertions(+), 136 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 6587421dde380..67a4559545dfb 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 505485db23a36..68ddba5f2415f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -312,6 +312,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. @@ -5759,9 +5766,15 @@ 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); + // Value number a phi definition + 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 // assumed for the memoryKind at the start "entryBlk". diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 11bce9e358e3d..f3c858bc9bcaa 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -161,11 +161,25 @@ 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); + // 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)) { unsigned newLclNum = iter->GetKey(); @@ -190,7 +204,15 @@ bool Compiler::optCopyProp( if (newLclDefVN != lclDefVN) { - continue; + bool arePhiDefsEquivalent = + varDefTreeIsPhiDefAtCycleEntry && vnStore->AreVNsEquivalent(lclDefVN, newLclDefVN); + if (!arePhiDefsEquivalent) + { + continue; + } + + JITDUMP("orig [%06u] 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 @@ -259,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); @@ -334,12 +374,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/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); } } } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 94407d3caf15c..f2c4d180cb054 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6718,6 +6718,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; @@ -10430,38 +10562,15 @@ 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); - - 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; - } - - JITDUMP(" Reachable through pred " FMT_BB "\n", predBlock->bbNum); - anyPredReachable = true; - break; - } - - if (!anyPredReachable) - { - JITDUMP(" " FMT_BB " was proven unreachable\n", block->bbNum); - vs.SetUnreachable(block); - } - } - - fgValueNumberBlock(block); + BasicBlock* const block = postOrder[i - 1]; + fgValueNumberBlocks(block, visitedBlocks); } #ifdef DEBUG @@ -10474,99 +10583,98 @@ PhaseStatus Compiler::fgValueNumber() return PhaseStatus::MODIFIED_EVERYTHING; } -void Compiler::fgValueNumberBlock(BasicBlock* blk) +//------------------------------------------------------------------------ +// 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) { - compCurBB = blk; + // Because we're not following the strict RPO, we may have already visisted this block. + // + if (BlockSetOps::IsMember(this, visitedBlocks, block->bbNum)) + { + return; + } - Statement* stmt = blk->firstStmt(); + JITDUMP("Visiting " FMT_BB "\n", block->bbNum); - // First: visit phis and check to see if all phi args have the same value. - for (; (stmt != nullptr) && stmt->IsPhiDefnStmt(); stmt = stmt->GetNextStmt()) + if (block != fgFirstBB) { - GenTreeLclVar* newSsaDef = stmt->GetRootNode()->AsLclVar(); - GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); - ValueNumPair phiVNP; - ValueNumPair sameVNP; - - for (GenTreePhi::Use& use : phiNode->Uses()) + bool anyPredReachable = false; + for (FlowEdge* pred = BlockPredsWithEH(block); pred != nullptr; pred = pred->getNextPredEdge()) { - GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); - if (!vnState->IsReachableThroughPred(blk, phiArg->gtPredBB)) + BasicBlock* predBlock = pred->getSourceBlock(); + if (!vnState->IsReachableThroughPred(block, predBlock)) { - 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"); + JITDUMP(" Unreachable through pred " FMT_BB "\n", predBlock->bbNum); + continue; } - 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); + JITDUMP(" Reachable through pred " FMT_BB "\n", predBlock->bbNum); + anyPredReachable = true; + break; + } - 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); - } - } + if (!anyPredReachable) + { + JITDUMP(" " FMT_BB " was proven unreachable\n", block->bbNum); + vnState->SetUnreachable(block); } + } - // We should have visited at least one phi arg in the loop above - assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); - assert(phiVNP.GetConservative() != ValueNumStore::NoVN); + fgValueNumberBlock(block); - ValueNumPair newSsaDefVNP; + // Mark block as visited + // + BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); - if (sameVNP.BothDefined()) + // 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; + }); + + // Re-value number all the PhiDefs in block + // + for (Statement* stmt = block->firstStmt(); (stmt != nullptr) && stmt->IsPhiDefnStmt(); + stmt = stmt->GetNextStmt()) { - // 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; + GenTreeLclVar* const newSsaDef = stmt->GetRootNode()->AsLclVar(); + fgValueNumberPhiDef(newSsaDef, block, /* isUpdate */ true); } - 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); - } + // TODO: Propagate those new VNs to their uses? + // + } +} - 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 +void Compiler::fgValueNumberBlock(BasicBlock* blk) +{ + compCurBB = blk; - newSsaDef->gtVNPair = vnStore->VNPForVoid(); - phiNode->gtVNPair = newSsaDefVNP; + Statement* stmt = blk->firstStmt(); + + // First: visit phis and check to see if all phi args have the same value. + for (; (stmt != nullptr) && stmt->IsPhiDefnStmt(); stmt = stmt->GetNextStmt()) + { + GenTreeLclVar* newSsaDef = stmt->GetRootNode()->AsLclVar(); + fgValueNumberPhiDef(newSsaDef, blk); } // Now do the same for each MemoryKind. @@ -10713,6 +10821,123 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) compCurBB = nullptr; } +//------------------------------------------------------------------------ +// 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; + 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) || 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) + { + // 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()); + +#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) + { + 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) 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)