From bcbaaf0e056360ca1c4c89dad9054d2ad9dd796d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Nov 2023 14:16:46 +0100 Subject: [PATCH 1/5] JIT: Sparse conditional propagation in value numbering Utilize the fact that VN is able to decide whether many non-trivial branches will be taken or not taken to prove basic blocks dynamically unreachable. Take this into account when building PHIs. Fix #94559 --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/valuenum.cpp | 79 ++++++++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 2c28b72bbaf4aa..45b162e1fbf63c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5057,6 +5057,7 @@ class Compiler // The value numbers for this compilation. ValueNumStore* vnStore; + struct ValueNumberState* vnVisitState; public: ValueNumStore* GetValueNumStore() diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index e3ca670a2c30fe..cb42333f5cf3b5 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9658,6 +9658,8 @@ struct ValueNumberState BVB_complete = 0x1, BVB_onAllDone = 0x2, BVB_onNotAllDone = 0x4, + // Set for statically reachable blocks that we have proven unreachable. + BVB_provenUnreachable = 0x8, }; bool GetVisitBit(unsigned bbNum, BlockVisitBits bvb) @@ -9787,7 +9789,8 @@ struct ValueNumberState JITDUMP(" Not yet completed.\n"); #endif // DEBUG_VN_VISIT - bool allPredsVisited = true; + bool allPredsVisited = true; + bool provenUnreachable = true; for (FlowEdge* pred = m_comp->BlockPredsWithEH(succ); pred != nullptr; pred = pred->getNextPredEdge()) { BasicBlock* predBlock = pred->getSourceBlock(); @@ -9796,6 +9799,12 @@ struct ValueNumberState allPredsVisited = false; break; } + + if (provenUnreachable && !GetVisitBit(predBlock->bbNum, BVB_provenUnreachable) && + IsReachableFromPred(predBlock, succ)) + { + provenUnreachable = false; + } } if (allPredsVisited) @@ -9808,6 +9817,11 @@ struct ValueNumberState assert(!GetVisitBit(succ->bbNum, BVB_onAllDone)); m_toDoAllPredsDone.Push(succ); SetVisitBit(succ->bbNum, BVB_onAllDone); + + if (provenUnreachable) + { + SetVisitBit(succ->bbNum, BVB_provenUnreachable); + } } else { @@ -9829,6 +9843,28 @@ struct ValueNumberState }); } + bool IsReachableFromPred(BasicBlock* predBlock, BasicBlock* block) + { + if (!predBlock->KindIs(BBJ_COND) || predBlock->JumpsToNext()) + { + return true; + } + + GenTree* lastTree = predBlock->lastStmt()->GetRootNode(); + assert(lastTree->OperIs(GT_JTRUE)); + + GenTree* cond = lastTree->gtGetOp1(); + ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Liberal)); + if (!m_comp->vnStore->IsVNConstant(normalVN)) + { + return true; + } + + bool isTaken = normalVN != m_comp->vnStore->VNZeroForType(TYP_INT); + BasicBlock* unreachableSucc = isTaken ? predBlock->Next() : predBlock->GetJumpDest(); + return block != unreachableSucc; + } + bool ToDoExists() { return m_toDoAllPredsDone.Size() > 0 || m_toDoNotAllPredsDone.Size() > 0; @@ -9965,6 +10001,8 @@ PhaseStatus Compiler::fgValueNumber() ValueNumberState vs(this); + vnVisitState = &vs; + // Push the first block. This has no preds. vs.m_toDoAllPredsDone.Push(fgFirstBB); @@ -9973,7 +10011,13 @@ PhaseStatus Compiler::fgValueNumber() while (vs.m_toDoAllPredsDone.Size() > 0) { BasicBlock* toDo = vs.m_toDoAllPredsDone.Pop(); + + // TODO-TP: We can skip VN'ing blocks we have proven unreachable + // here. However, currently downstream phases are not prepared to + // handle the fact that some blocks that are seemingly reachable + // have not been VN'd. fgValueNumberBlock(toDo); + // Record that we've visited "toDo", and add successors to the right sets. vs.FinishVisit(toDo); } @@ -10008,10 +10052,11 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) { compCurBB = blk; + bool isReachableBlock = !vnVisitState->GetVisitBit(blk->bbNum, ValueNumberState::BVB_provenUnreachable); + Statement* stmt = blk->firstStmt(); - // First: visit phi's. If "newVNForPhis", give them new VN's. If not, - // first check to see if all phi args have the same value. + // 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(); @@ -10021,9 +10066,17 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) for (GenTreePhi::Use& use : phiNode->Uses()) { - GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); - ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); - ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; + GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); + if (isReachableBlock && + vnVisitState->GetVisitBit(phiArg->gtPredBB->bbNum, ValueNumberState::BVB_provenUnreachable)) + { + JITDUMP(" Skipping phi arg [%06u]; pred " FMT_BB " was proven unreachable\n", dspTreeID(phiArg), + phiArg->gtPredBB->bbNum); + continue; + } + + ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); + ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; phiArg->gtVNPair = phiArgVNP; @@ -10049,12 +10102,12 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) } } -#ifdef DEBUG - // There should be at least to 2 PHI arguments so phiVN's VNs should always be VNF_Phi functions. - VNFuncApp phiFunc; - assert(vnStore->GetVNFunc(phiVNP.GetLiberal(), &phiFunc) && (phiFunc.m_func == VNF_Phi)); - assert(vnStore->GetVNFunc(phiVNP.GetConservative(), &phiFunc) && (phiFunc.m_func == VNF_Phi)); -#endif + // We should have visited at least one phi arg in the loop above + // 1. For reachable blocks, at least one pred should be reachable or we would have proven this block to be + // unreachable too + // 2. For unreachable blocks we build for all phi args. + assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); + assert(phiVNP.GetConservative() != ValueNumStore::NoVN); ValueNumPair newSsaDefVNP; @@ -10117,6 +10170,8 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) else { // Are all the VN's the same? + // TODO-CQ: Skip proven unreachable blocks. Do we have any way to look up the pred block for memory + // PHIs? BasicBlock::MemoryPhiArg* phiArgs = blk->bbMemorySsaPhiFunc[memoryKind]; assert(phiArgs != BasicBlock::EmptyMemoryPhiDef); // There should be > 1 args to a phi. From 4b259e95fa22a6f03304c4e04fec96cf578c5fbb Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Nov 2023 21:45:29 +0100 Subject: [PATCH 2/5] Handle statically unreachable preds The assert that the phi loop found a phi could be hit in cases where a block had a statically unreachable pred. In that case SSA may not have added any PHI arg for that pred, and if we then proved all other preds unreachable, we would end up with no VN for a phi. Fix it by using the value of the last phi arg in those cases. --- src/coreclr/jit/valuenum.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index cb42333f5cf3b5..3cc098fe95fb29 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9854,7 +9854,7 @@ struct ValueNumberState assert(lastTree->OperIs(GT_JTRUE)); GenTree* cond = lastTree->gtGetOp1(); - ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Liberal)); + ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Conservative)); if (!m_comp->vnStore->IsVNConstant(normalVN)) { return true; @@ -10052,8 +10052,6 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) { compCurBB = blk; - bool isReachableBlock = !vnVisitState->GetVisitBit(blk->bbNum, ValueNumberState::BVB_provenUnreachable); - Statement* stmt = blk->firstStmt(); // First: visit phis and check to see if all phi args have the same value. @@ -10067,12 +10065,17 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) for (GenTreePhi::Use& use : phiNode->Uses()) { GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); - if (isReachableBlock && - vnVisitState->GetVisitBit(phiArg->gtPredBB->bbNum, ValueNumberState::BVB_provenUnreachable)) + if (vnVisitState->GetVisitBit(phiArg->gtPredBB->bbNum, ValueNumberState::BVB_provenUnreachable)) { - JITDUMP(" Skipping phi arg [%06u]; pred " FMT_BB " was proven unreachable\n", dspTreeID(phiArg), + JITDUMP(" Phi arg [%06u] refers to proven unreachable pred " FMT_BB "\n", dspTreeID(phiArg), phiArg->gtPredBB->bbNum); - continue; + + if ((use.GetNext() != nullptr) || (phiVNP.GetLiberal() != ValueNumStore::NoVN)) + { + continue; + } + + JITDUMP(" ..but it looks like all preds are unreachable, so we are using it anyway\n"); } ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); @@ -10103,9 +10106,6 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) } // We should have visited at least one phi arg in the loop above - // 1. For reachable blocks, at least one pred should be reachable or we would have proven this block to be - // unreachable too - // 2. For unreachable blocks we build for all phi args. assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); assert(phiVNP.GetConservative() != ValueNumStore::NoVN); @@ -10170,8 +10170,6 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) else { // Are all the VN's the same? - // TODO-CQ: Skip proven unreachable blocks. Do we have any way to look up the pred block for memory - // PHIs? BasicBlock::MemoryPhiArg* phiArgs = blk->bbMemorySsaPhiFunc[memoryKind]; assert(phiArgs != BasicBlock::EmptyMemoryPhiDef); // There should be > 1 args to a phi. From 84d764ca79a8f9aec58321ecd240e24da81e81ed Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Nov 2023 21:59:27 +0100 Subject: [PATCH 3/5] Go back to liberal VNs --- src/coreclr/jit/valuenum.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 3cc098fe95fb29..b2860e9fceca24 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9854,7 +9854,9 @@ struct ValueNumberState assert(lastTree->OperIs(GT_JTRUE)); GenTree* cond = lastTree->gtGetOp1(); - ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Conservative)); + // TODO-Cleanup: Using liberal VNs here is a bit questionable as it add + // a cross-phase dependency on RBO to definitely fold this branch away. + ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Liberal)); if (!m_comp->vnStore->IsVNConstant(normalVN)) { return true; From bc51c3c0457652daca5a589d244e1c4961a79584 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Nov 2023 22:25:51 +0100 Subject: [PATCH 4/5] Run jit-format --- src/coreclr/jit/valuenum.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index b2860e9fceca24..4aa02cb77a43db 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9853,7 +9853,7 @@ struct ValueNumberState GenTree* lastTree = predBlock->lastStmt()->GetRootNode(); assert(lastTree->OperIs(GT_JTRUE)); - GenTree* cond = lastTree->gtGetOp1(); + GenTree* cond = lastTree->gtGetOp1(); // TODO-Cleanup: Using liberal VNs here is a bit questionable as it add // a cross-phase dependency on RBO to definitely fold this branch away. ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Liberal)); From bde2d366716d82b5c095b9dc294831b6739f7937 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Nov 2023 22:30:30 +0100 Subject: [PATCH 5/5] Nit --- src/coreclr/jit/valuenum.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 4aa02cb77a43db..4e804aba7f4e7b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9854,8 +9854,9 @@ struct ValueNumberState assert(lastTree->OperIs(GT_JTRUE)); GenTree* cond = lastTree->gtGetOp1(); - // TODO-Cleanup: Using liberal VNs here is a bit questionable as it add - // a cross-phase dependency on RBO to definitely fold this branch away. + // TODO-Cleanup: Using liberal VNs here is a bit questionable as it + // adds a cross-phase dependency on RBO to definitely fold this branch + // away. ValueNum normalVN = m_comp->vnStore->VNNormalValue(cond->GetVN(VNK_Liberal)); if (!m_comp->vnStore->IsVNConstant(normalVN)) {