From c1d159ab1090ebbf6557f576455ac30d83334f03 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 2 Jul 2024 20:07:35 -0700 Subject: [PATCH 1/5] proof of concept --- src/coreclr/jit/assertionprop.cpp | 78 +++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 3a3139db4a157..b235b99e282d1 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2318,6 +2318,82 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) return NO_ASSERTION_INDEX; } + // If this is a loop header (cycle entry), see if this phi-def + // is value equivalent to some other phi-def in the block. Note + // value numbering is a one-pass forward algorithm and doesn't + // know the value numbers for backege phis, so cycle entry + // phis will all have novel VNs. + // + // We walk "backwards" so that + // we don't generate the assertion too early...? + // + // Consider: compute hash of forward edge phi inputs during VN, + // so we can quickly rule out inequivalent cases. + // + // Consider: tap into reachability logic available to VN, and/or + // edit out PHIs from impossible preds, or mark the phi nodes + // in some way so we can ignore them here. + // + FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(compCurBB); + if ((loop != nullptr) && (loop->GetHeader() == compCurBB)) + { + for (Statement* stmt = compCurBB->firstStmt(); stmt != compCurStmt; stmt = stmt->GetNextStmt()) + { + // Consider: limit searching if there are lots of phi defs + // + GenTree* const otherTree = stmt->GetRootNode(); + assert(otherTree->IsPhiDefn()); + + if (otherTree->TypeGet() != tree->TypeGet()) + { + continue; + } + + GenTreePhi* const treePhi = tree->AsLclVar()->Data()->AsPhi(); + GenTreePhi* const otherPhi = otherTree->AsLclVar()->Data()->AsPhi(); + GenTreePhi::UseIterator treeIter = treePhi->Uses().begin(); + GenTreePhi::UseIterator treeEnd = treePhi->Uses().end(); + GenTreePhi::UseIterator otherIter = otherPhi->Uses().begin(); + GenTreePhi::UseIterator otherEnd = otherPhi->Uses().end(); + + bool phiDefsAreEquivalent = true; + + for (; (treeIter != treeEnd) && (otherIter != otherEnd); ++treeIter, ++otherIter) + { + GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg(); + GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg(); + + assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB); + + // Consult SSA defs always... + // + ValueNum treePhiArgVN = + lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum())->m_vnPair.GetConservative(); + ValueNum otherPhiArgVN = + lvaGetDesc(otherPhiArg)->GetPerSsaData(otherPhiArg->GetSsaNum())->m_vnPair.GetConservative(); + + // Consider: see if we already think these two VNs are equivalent.. + // + if (treePhiArgVN != otherPhiArgVN) + { + phiDefsAreEquivalent = false; + break; + } + } + + // If we didn't verify all phi args we have failed to prove equivalence + // + phiDefsAreEquivalent &= (treeIter == treeEnd); + phiDefsAreEquivalent &= (otherIter == otherEnd); + + if (phiDefsAreEquivalent) + { + JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi), dspTreeID(otherPhi)); + // generate assertion! + } + } + } + // Try to find if all phi arguments are known to be non-null. bool isNonNull = true; for (GenTreePhi::Use& use : tree->AsLclVar()->Data()->AsPhi()->Uses()) @@ -6603,6 +6679,8 @@ PhaseStatus Compiler::optAssertionPropMain() } } + compCurStmt = stmt; + // Perform assertion gen for control flow based assertions. for (GenTree* const tree : stmt->TreeList()) { From d29cead526332a2c843a34efc775d39df3a060de Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 3 Jul 2024 19:57:47 -0700 Subject: [PATCH 2/5] fatal flaw -- no copy prop during global AP --- src/coreclr/jit/assertionprop.cpp | 54 +++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index b235b99e282d1..70016d516f008 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1184,6 +1184,28 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, assertion.op2.SetIconFlag(GTF_EMPTY); } // + // Are two phi defs are equivalent? + // + else if (op1->IsPhiDefn() && op2->IsPhiDefn()) + { + GenTreeLclVarCommon* const op1Lcl = op1->AsLclVar(); + GenTreeLclVarCommon* const op2Lcl = op2->AsLclVar(); + GenTreePhi* const op1Phi = op1Lcl->Data()->AsPhi(); + GenTreePhi* const op2Phi = op2Lcl->Data()->AsPhi(); + + assertion.op1.kind = O1K_LCLVAR; + assertion.op1.lcl.lclNum = op1Lcl->GetLclNum(); + assertion.op1.lcl.ssaNum = op1Lcl->GetSsaNum(); + assertion.op1.vn = optConservativeNormalVN(op1Phi); + + assertion.op2.kind = O2K_LCLVAR_COPY; + assertion.op2.lcl.lclNum = op2Lcl->GetLclNum(); + assertion.op2.lcl.ssaNum = op2Lcl->GetSsaNum(); + assertion.op2.vn = optConservativeNormalVN(op2Phi); + + assertion.assertionKind = assertionKind; + } + // // Are we making an assertion about a local variable? // else if (op1->OperIsScalarLocal()) @@ -1433,7 +1455,6 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, } } } - // // Are we making an IsType assertion? // @@ -2321,12 +2342,16 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) // If this is a loop header (cycle entry), see if this phi-def // is value equivalent to some other phi-def in the block. Note // value numbering is a one-pass forward algorithm and doesn't - // know the value numbers for backege phis, so cycle entry + // know the value numbers for back edge phi args, so cycle entry // phis will all have novel VNs. // // We walk "backwards" so that // we don't generate the assertion too early...? // + // Note we can create at most one assertion, hopefully they chain + // so if one phi-def is both non-null and equivalent to another, + // the first generates the non-null assertion, the second the eq assertion. + // // Consider: compute hash of forward edge phi inputs during VN, // so we can quickly rule out inequivalent cases. // @@ -2334,6 +2359,8 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) // edit out PHIs from impossible preds, or mark the phi nodes // in some way so we can ignore them here. // + // Consider: might want to skip handler entries? + // FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(compCurBB); if ((loop != nullptr) && (loop->GetHeader() == compCurBB)) { @@ -2363,9 +2390,15 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg(); GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg(); + // Consider: ignore phi if gtPredBB is unreachable. + // But we don't know that here. + // assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB); - // Consult SSA defs always... + // Consult SSA defs always... in principle we could just do this for + // back edge PhiArgs and use the Phi Arg VNs for forward edges, + // but the loops are invalidated by RBO so we're not sure which is which. + // Possibly we could rely on DFS instead. // ValueNum treePhiArgVN = lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum())->m_vnPair.GetConservative(); @@ -2376,6 +2409,7 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) // if (treePhiArgVN != otherPhiArgVN) { + // Consider: in some cases we know hte PHIs can't be equivalent... phiDefsAreEquivalent = false; break; } @@ -2389,7 +2423,7 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) if (phiDefsAreEquivalent) { JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi), dspTreeID(otherPhi)); - // generate assertion! + return optCreateAssertion(tree, otherTree, OAK_EQUAL); } } } @@ -2398,7 +2432,12 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) bool isNonNull = true; for (GenTreePhi::Use& use : tree->AsLclVar()->Data()->AsPhi()->Uses()) { - if (!vnStore->IsKnownNonNull(use.GetNode()->gtVNPair.GetConservative())) + // Use same trick as above to get VNs for back edge phi args + // + GenTreePhiArg* const treePhiArg = use.GetNode()->AsPhiArg(); + ValueNum treePhiArgVN = + lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum())->m_vnPair.GetConservative(); + if (!vnStore->IsKnownNonNull(treePhiArgVN)) { isNonNull = false; break; @@ -6764,7 +6803,10 @@ PhaseStatus Compiler::optAssertionPropMain() fgRemoveRestOfBlock = false; // Walk the statement trees in this basic block - Statement* stmt = block->FirstNonPhiDef(); + // (remember if we generated equiv phi defs? + // + // Statement* stmt = block->FirstNonPhiDef(); + Statement* stmt = block->firstStmt(); while (stmt != nullptr) { // Propagation tells us to remove the rest of the block. Remove it. From e6ecd600f82b87aa12ac00b969e4074bbc1d6a6a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 4 Jul 2024 19:39:25 -0700 Subject: [PATCH 3/5] move to const prop --- src/coreclr/jit/assertionprop.cpp | 121 +----------------------------- src/coreclr/jit/copyprop.cpp | 111 +++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 126 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 70016d516f008..885bc13a7bac5 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1184,28 +1184,6 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, assertion.op2.SetIconFlag(GTF_EMPTY); } // - // Are two phi defs are equivalent? - // - else if (op1->IsPhiDefn() && op2->IsPhiDefn()) - { - GenTreeLclVarCommon* const op1Lcl = op1->AsLclVar(); - GenTreeLclVarCommon* const op2Lcl = op2->AsLclVar(); - GenTreePhi* const op1Phi = op1Lcl->Data()->AsPhi(); - GenTreePhi* const op2Phi = op2Lcl->Data()->AsPhi(); - - assertion.op1.kind = O1K_LCLVAR; - assertion.op1.lcl.lclNum = op1Lcl->GetLclNum(); - assertion.op1.lcl.ssaNum = op1Lcl->GetSsaNum(); - assertion.op1.vn = optConservativeNormalVN(op1Phi); - - assertion.op2.kind = O2K_LCLVAR_COPY; - assertion.op2.lcl.lclNum = op2Lcl->GetLclNum(); - assertion.op2.lcl.ssaNum = op2Lcl->GetSsaNum(); - assertion.op2.vn = optConservativeNormalVN(op2Phi); - - assertion.assertionKind = assertionKind; - } - // // Are we making an assertion about a local variable? // else if (op1->OperIsScalarLocal()) @@ -2339,101 +2317,11 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) return NO_ASSERTION_INDEX; } - // If this is a loop header (cycle entry), see if this phi-def - // is value equivalent to some other phi-def in the block. Note - // value numbering is a one-pass forward algorithm and doesn't - // know the value numbers for back edge phi args, so cycle entry - // phis will all have novel VNs. - // - // We walk "backwards" so that - // we don't generate the assertion too early...? - // - // Note we can create at most one assertion, hopefully they chain - // so if one phi-def is both non-null and equivalent to another, - // the first generates the non-null assertion, the second the eq assertion. - // - // Consider: compute hash of forward edge phi inputs during VN, - // so we can quickly rule out inequivalent cases. - // - // Consider: tap into reachability logic available to VN, and/or - // edit out PHIs from impossible preds, or mark the phi nodes - // in some way so we can ignore them here. - // - // Consider: might want to skip handler entries? - // - FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(compCurBB); - if ((loop != nullptr) && (loop->GetHeader() == compCurBB)) - { - for (Statement* stmt = compCurBB->firstStmt(); stmt != compCurStmt; stmt = stmt->GetNextStmt()) - { - // Consider: limit searching if there are lots of phi defs - // - GenTree* const otherTree = stmt->GetRootNode(); - assert(otherTree->IsPhiDefn()); - - if (otherTree->TypeGet() != tree->TypeGet()) - { - continue; - } - - GenTreePhi* const treePhi = tree->AsLclVar()->Data()->AsPhi(); - GenTreePhi* const otherPhi = otherTree->AsLclVar()->Data()->AsPhi(); - GenTreePhi::UseIterator treeIter = treePhi->Uses().begin(); - GenTreePhi::UseIterator treeEnd = treePhi->Uses().end(); - GenTreePhi::UseIterator otherIter = otherPhi->Uses().begin(); - GenTreePhi::UseIterator otherEnd = otherPhi->Uses().end(); - - bool phiDefsAreEquivalent = true; - - for (; (treeIter != treeEnd) && (otherIter != otherEnd); ++treeIter, ++otherIter) - { - GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg(); - GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg(); - - // Consider: ignore phi if gtPredBB is unreachable. - // But we don't know that here. - // - assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB); - - // Consult SSA defs always... in principle we could just do this for - // back edge PhiArgs and use the Phi Arg VNs for forward edges, - // but the loops are invalidated by RBO so we're not sure which is which. - // Possibly we could rely on DFS instead. - // - ValueNum treePhiArgVN = - lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum())->m_vnPair.GetConservative(); - ValueNum otherPhiArgVN = - lvaGetDesc(otherPhiArg)->GetPerSsaData(otherPhiArg->GetSsaNum())->m_vnPair.GetConservative(); - - // Consider: see if we already think these two VNs are equivalent.. - // - if (treePhiArgVN != otherPhiArgVN) - { - // Consider: in some cases we know hte PHIs can't be equivalent... - phiDefsAreEquivalent = false; - break; - } - } - - // If we didn't verify all phi args we have failed to prove equivalence - // - phiDefsAreEquivalent &= (treeIter == treeEnd); - phiDefsAreEquivalent &= (otherIter == otherEnd); - - if (phiDefsAreEquivalent) - { - JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi), dspTreeID(otherPhi)); - return optCreateAssertion(tree, otherTree, OAK_EQUAL); - } - } - } - // Try to find if all phi arguments are known to be non-null. + // bool isNonNull = true; for (GenTreePhi::Use& use : tree->AsLclVar()->Data()->AsPhi()->Uses()) { - // Use same trick as above to get VNs for back edge phi args - // GenTreePhiArg* const treePhiArg = use.GetNode()->AsPhiArg(); ValueNum treePhiArgVN = lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum())->m_vnPair.GetConservative(); @@ -6718,8 +6606,6 @@ PhaseStatus Compiler::optAssertionPropMain() } } - compCurStmt = stmt; - // Perform assertion gen for control flow based assertions. for (GenTree* const tree : stmt->TreeList()) { @@ -6803,10 +6689,7 @@ PhaseStatus Compiler::optAssertionPropMain() fgRemoveRestOfBlock = false; // Walk the statement trees in this basic block - // (remember if we generated equiv phi defs? - // - // Statement* stmt = block->FirstNonPhiDef(); - Statement* stmt = block->firstStmt(); + Statement* stmt = block->FirstNonPhiDef(); while (stmt != nullptr) { // Propagation tells us to remove the rest of the block. Remove it. diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 11bce9e358e3d..4beacc015e0ae 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -161,9 +161,12 @@ 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 lclDefVN = varSsaDsc->m_vnPair.GetConservative(); assert(lclDefVN != ValueNumStore::NoVN); for (LclNumToLiveDefsMap::Node* const iter : LclNumToLiveDefsMap::KeyValueIteration(curSsaName)) @@ -190,7 +193,85 @@ bool Compiler::optCopyProp( if (newLclDefVN != lclDefVN) { - continue; + // If the definition is a phi def, and is defined in a block that has a backedge, + // we may be able to prove equivalence with some other phi def in that block, despite + // the fact that the VNs do not match. + // + // This happens because VN is one-pass and doesn't know the VNs for backedge PhiArgs, + // so the VNs for phi defs for loop entry blocks is always a novel opaque VN. But + // we can query those backedge VNs after VN is done, and if all Phi Arg VNs match, then + // the two Phi Defs are equivalent and could have had the same VN. + // + bool foundEquivalentPhi = false; + + if (varDefBlock == newLclSsaDef->GetBlock()) + { + GenTreeLclVarCommon* const otherTree = newLclSsaDef->GetDefNode(); + + // Do we have phi definitions? Do the types match? + // + if ((otherTree != nullptr) && otherTree->IsPhiDefn() && (varDefTree != nullptr) && + varDefTree->IsPhiDefn() && (otherTree->TypeGet() == tree->TypeGet())) + { + // Is the block a cycle entry? + // + FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(varDefBlock); + if ((loop != nullptr) && (loop->GetHeader() == varDefBlock)) + { + // Are all the Phi Args VN equivalent? + // + GenTreePhi* const treePhi = varDefTree->AsLclVar()->Data()->AsPhi(); + GenTreePhi* const otherPhi = otherTree->AsLclVar()->Data()->AsPhi(); + GenTreePhi::UseIterator treeIter = treePhi->Uses().begin(); + GenTreePhi::UseIterator treeEnd = treePhi->Uses().end(); + GenTreePhi::UseIterator otherIter = otherPhi->Uses().begin(); + GenTreePhi::UseIterator otherEnd = otherPhi->Uses().end(); + + bool phiArgsAreEquivalent = true; + + for (; (treeIter != treeEnd) && (otherIter != otherEnd); ++treeIter, ++otherIter) + { + GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg(); + GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg(); + + assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB); + + // Consult SSA defs always... in principle we could just do this for + // back edge PhiArgs and use the Phi Arg VNs for forward edges. + // + ValueNum treePhiArgVN = lvaGetDesc(treePhiArg) + ->GetPerSsaData(treePhiArg->GetSsaNum()) + ->m_vnPair.GetConservative(); + ValueNum otherPhiArgVN = lvaGetDesc(otherPhiArg) + ->GetPerSsaData(otherPhiArg->GetSsaNum()) + ->m_vnPair.GetConservative(); + + if (treePhiArgVN != otherPhiArgVN) + { + phiArgsAreEquivalent = false; + break; + } + } + + // If we didn't verify all phi args we have failed to prove equivalence + // + phiArgsAreEquivalent &= (treeIter == treeEnd); + phiArgsAreEquivalent &= (otherIter == otherEnd); + + if (phiArgsAreEquivalent) + { + JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi), + dspTreeID(otherPhi)); + foundEquivalentPhi = true; + } + } + } + } + + if (!foundEquivalentPhi) + { + continue; + } } // It may not be profitable to propagate a 'doNotEnregister' lclVar to an existing use of an @@ -257,8 +338,24 @@ bool Compiler::optCopyProp( unsigned newSsaNum = newLclVarDsc->GetSsaNumForSsaDef(newLclSsaDef); assert(newSsaNum != SsaConfig::RESERVED_SSA_NUM); - tree->AsLclVarCommon()->SetLclNum(newLclNum); - tree->AsLclVarCommon()->SetSsaNum(newSsaNum); + tree->SetLclNum(newLclNum); + tree->SetSsaNum(newSsaNum); + + // Update VN to match. + // this is a bit of hack. + // can't just do this in isolation, sigh... + // + if (newLclDefVN != lclDefVN) + { + tree->SetVNs(newLclSsaDef->m_vnPair); + GenTree* const parent = tree->gtGetParent(nullptr); + + if (parent->OperIs(GT_COMMA)) + { + parent->SetVNs(newLclSsaDef->m_vnPair); + } + } + gtUpdateSideEffects(stmt, tree); newLclSsaDef->AddUse(block); @@ -337,7 +434,7 @@ void Compiler::optCopyPropPushDef(GenTree* defNode, GenTreeLclVarCommon* lclNode if ((defNode != nullptr) && defNode->IsPhiDefn()) { // TODO-CQ: design better heuristics for propagation and remove this. - ssaNum = SsaConfig::RESERVED_SSA_NUM; + // ssaNum = SsaConfig::RESERVED_SSA_NUM; } pushDef(lclNum, ssaNum); From dd66fff7f90fe68d8a0d348052ddcb6d1cb5ca30 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 8 Jul 2024 11:32:48 -0700 Subject: [PATCH 4/5] rework to improve TP --- src/coreclr/jit/copyprop.cpp | 146 ++++++++++++++++++++++++----------- 1 file changed, 101 insertions(+), 45 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 4beacc015e0ae..cc2744e943d57 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -169,6 +169,15 @@ bool Compiler::optCopyProp( ValueNum lclDefVN = varSsaDsc->m_vnPair.GetConservative(); assert(lclDefVN != ValueNumStore::NoVN); + 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(); @@ -204,70 +213,114 @@ bool Compiler::optCopyProp( // bool foundEquivalentPhi = false; - if (varDefBlock == newLclSsaDef->GetBlock()) + // Is the new local def from the same block? + // + if (varDefTreeIsPhiDefAtCycleEntry && (varDefBlock == newLclSsaDef->GetBlock())) { GenTreeLclVarCommon* const otherTree = newLclSsaDef->GetDefNode(); - // Do we have phi definitions? Do the types match? + // Is it also a phi definition? Do the types match? // - if ((otherTree != nullptr) && otherTree->IsPhiDefn() && (varDefTree != nullptr) && - varDefTree->IsPhiDefn() && (otherTree->TypeGet() == tree->TypeGet())) + if ((otherTree != nullptr) && otherTree->IsPhiDefn() && (otherTree->TypeGet() == tree->TypeGet())) { - // Is the block a cycle entry? + // Are all the Phi Args VN equivalent? // - FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(varDefBlock); - if ((loop != nullptr) && (loop->GetHeader() == varDefBlock)) + GenTreePhi* const treePhi = varDefTree->AsLclVar()->Data()->AsPhi(); + GenTreePhi* const otherPhi = otherTree->AsLclVar()->Data()->AsPhi(); + GenTreePhi::UseIterator treeIter = treePhi->Uses().begin(); + GenTreePhi::UseIterator treeEnd = treePhi->Uses().end(); + GenTreePhi::UseIterator otherIter = otherPhi->Uses().begin(); + GenTreePhi::UseIterator otherEnd = otherPhi->Uses().end(); + + bool phiArgsAreEquivalent = true; + + for (; (treeIter != treeEnd) && (otherIter != otherEnd); ++treeIter, ++otherIter) { - // Are all the Phi Args VN equivalent? - // - GenTreePhi* const treePhi = varDefTree->AsLclVar()->Data()->AsPhi(); - GenTreePhi* const otherPhi = otherTree->AsLclVar()->Data()->AsPhi(); - GenTreePhi::UseIterator treeIter = treePhi->Uses().begin(); - GenTreePhi::UseIterator treeEnd = treePhi->Uses().end(); - GenTreePhi::UseIterator otherIter = otherPhi->Uses().begin(); - GenTreePhi::UseIterator otherEnd = otherPhi->Uses().end(); + GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg(); + GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg(); + + assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB); - bool phiArgsAreEquivalent = true; + ValueNum treePhiArgVN = treePhiArg->gtVNPair.GetConservative(); + ValueNum otherPhiArgVN = otherPhiArg->gtVNPair.GetConservative(); - for (; (treeIter != treeEnd) && (otherIter != otherEnd); ++treeIter, ++otherIter) + const bool phiArgIsFromDfsBackedge = m_dfsTree->IsAncestor(varDefBlock, treePhiArg->gtPredBB); + + if ((treePhiArgVN == ValueNumStore::NoVN) && phiArgIsFromDfsBackedge) { - GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg(); - GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg(); + // Consult SSA defs + // + LclSsaVarDsc* const treePhiArgSsaDef = + lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum()); + ValueNum treePhiArgSsaDefVN = treePhiArgSsaDef->m_vnPair.GetConservative(); + + // Opportunisitcally update the PhiArgVNs, both to short-circuit future checks here + // in copy prop, and help later phases that also scan phi arg lists (eg RBO). + // (consider doing this as a VN post pass?) + // + if (treePhiArgSsaDefVN != treePhiArgVN) + { + JITDUMP("Updating backedge phi arg [%06u] vn to " FMT_VN "\n", dspTreeID(treePhiArg), + treePhiArgSsaDefVN); + treePhiArg->SetVNs(treePhiArgSsaDef->m_vnPair); + } + + treePhiArgVN = treePhiArgSsaDefVN; + } - assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB); + if ((otherPhiArgVN == ValueNumStore::NoVN) && phiArgIsFromDfsBackedge) + { + LclSsaVarDsc* const otherPhiArgSsaDef = + lvaGetDesc(otherPhiArg)->GetPerSsaData(otherPhiArg->GetSsaNum()); + ValueNum otherPhiArgSsaDefVN = otherPhiArgSsaDef->m_vnPair.GetConservative(); - // Consult SSA defs always... in principle we could just do this for - // back edge PhiArgs and use the Phi Arg VNs for forward edges. + // Opportunisitcally update the PhiArgVNs, both to short-circuit future checks here + // in copy prop, and help later phases that also scan phi arg lists (eg RBO). + // (consider doing this as a VN post pass?) // - ValueNum treePhiArgVN = lvaGetDesc(treePhiArg) - ->GetPerSsaData(treePhiArg->GetSsaNum()) - ->m_vnPair.GetConservative(); - ValueNum otherPhiArgVN = lvaGetDesc(otherPhiArg) - ->GetPerSsaData(otherPhiArg->GetSsaNum()) - ->m_vnPair.GetConservative(); - - if (treePhiArgVN != otherPhiArgVN) + if (otherPhiArgSsaDefVN != otherPhiArgVN) { - phiArgsAreEquivalent = false; - break; + JITDUMP("Updating backedge phi arg [%06u] vn to " FMT_VN "\n", dspTreeID(otherPhiArg), + otherPhiArgSsaDefVN); + otherPhiArg->SetVNs(otherPhiArgSsaDef->m_vnPair); } + + otherPhiArgVN = otherPhiArgSsaDefVN; } - // If we didn't verify all phi args we have failed to prove equivalence + // If the updated VNs differ, the phis are not equivalent // - phiArgsAreEquivalent &= (treeIter == treeEnd); - phiArgsAreEquivalent &= (otherIter == otherEnd); + if (treePhiArgVN != otherPhiArgVN) + { + phiArgsAreEquivalent = false; + break; + } - if (phiArgsAreEquivalent) + // If we failed to find meaningful VNs, the phis are not equivalent + // + if (treePhiArgVN == ValueNumStore::NoVN) { - JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi), - dspTreeID(otherPhi)); - foundEquivalentPhi = true; + phiArgsAreEquivalent = false; + break; } } + + // If we didn't verify all phi args we have failed to prove equivalence + // + phiArgsAreEquivalent &= (treeIter == treeEnd); + phiArgsAreEquivalent &= (otherIter == otherEnd); + + if (phiArgsAreEquivalent) + { + JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi), + dspTreeID(otherPhi)); + foundEquivalentPhi = true; + } } } + // VNs differed, and we didn't find phis equivalent, move on to the next candidate. + // if (!foundEquivalentPhi) { continue; @@ -341,18 +394,21 @@ bool Compiler::optCopyProp( tree->SetLclNum(newLclNum); tree->SetSsaNum(newSsaNum); - // Update VN to match. - // this is a bit of hack. - // can't just do this in isolation, sigh... + // 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* const parent = tree->gtGetParent(nullptr); + GenTree* parent = tree->gtGetParent(nullptr); - if (parent->OperIs(GT_COMMA)) + while ((parent != nullptr) && parent->OperIs(GT_COMMA)) { - parent->SetVNs(newLclSsaDef->m_vnPair); + 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); } } From f510faebdb82f0404a3576c76661cb910b1d5898 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 8 Jul 2024 14:33:23 -0700 Subject: [PATCH 5/5] revert AP changes, cleanup --- src/coreclr/jit/assertionprop.cpp | 7 ++----- src/coreclr/jit/copyprop.cpp | 6 ------ 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 885bc13a7bac5..3a3139db4a157 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1433,6 +1433,7 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, } } } + // // Are we making an IsType assertion? // @@ -2318,14 +2319,10 @@ AssertionIndex Compiler::optAssertionGenPhiDefn(GenTree* tree) } // Try to find if all phi arguments are known to be non-null. - // bool isNonNull = true; for (GenTreePhi::Use& use : tree->AsLclVar()->Data()->AsPhi()->Uses()) { - GenTreePhiArg* const treePhiArg = use.GetNode()->AsPhiArg(); - ValueNum treePhiArgVN = - lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum())->m_vnPair.GetConservative(); - if (!vnStore->IsKnownNonNull(treePhiArgVN)) + if (!vnStore->IsKnownNonNull(use.GetNode()->gtVNPair.GetConservative())) { isNonNull = false; break; diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index cc2744e943d57..97bf549b5877b 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -487,12 +487,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); } }