From e971c8f117ddd9e046ac62a8bac04180060fdd40 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 Jul 2024 15:37:57 +0200 Subject: [PATCH 01/10] Introduce VNWalkPhis and use in VNNeverNegative as an example --- src/coreclr/jit/valuenum.cpp | 108 ++++++++++++++++++++++++++++++++++- src/coreclr/jit/valuenum.h | 8 +++ 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 7e00b61aabb25b..5f12220f1c9671 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -1635,7 +1635,20 @@ bool ValueNumStore::IsKnownNonNull(ValueNum vn) } VNFuncApp funcAttr; - return GetVNFunc(vn, &funcAttr) && (s_vnfOpAttribs[funcAttr.m_func] & VNFOA_KnownNonNull) != 0; + if (!GetVNFunc(vn, &funcAttr)) + { + return false; + } + + if ((s_vnfOpAttribs[funcAttr.m_func] & VNFOA_KnownNonNull) != 0) + { + return true; + } + + // TODO: we can recognize more non-null idioms here, e.g. + // ADD(IsKnownNonNull(op1), smallCns), etc. + + return false; } bool ValueNumStore::IsSharedStatic(ValueNum vn) @@ -6483,6 +6496,20 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) #endif #endif // FEATURE_HW_INTRINSICS + case VNF_PhiDef: + { + // Inspect all PHI args (call IsVNNeverNegative for them) + return VNWalkPhis(funcApp, [](Compiler* comp, ValueNum phiVN) -> PhiWalkResult { + // Bail out if the type is not integral + if (!varTypeIsIntegral(comp->vnStore->TypeOfVN(phiVN))) + { + return PhiWalkResult::Abort; + } + + return comp->vnStore->IsVNNeverNegative(phiVN) ? PhiWalkResult::Continue : PhiWalkResult::Abort; + }); + } + default: break; } @@ -15034,3 +15061,82 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) } } } + +//-------------------------------------------------------------------------------- +// VNWalkPhis: Walk the given PhiDef and call a callback on each Phi argument (its VN). +// +// Arguments: +// phiDefFunc - VNF_PhiDef function +// walkPhiVnsFn - The callback function to call on each Phi argument. +// maxDepth - Maximum depth to handle recursive PhiDef chains. +// +// Return Value: +// true if the walk was successful, false if the walk was aborted. +// +bool ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, int maxDepth) +{ + assert(maxDepth > 0); + assert(phiDefFunc.m_func == VNF_PhiDef); + + // VNF_PhiDef(LclNum, SsaNum, VNF_Phi) + unsigned lclNum = phiDefFunc.m_args[0]; + ValueNum nextPhiArg = phiDefFunc.m_args[2]; + + if ((lclNum == static_cast(-1)) || (nextPhiArg == NoVN)) + { + return false; + } + + // Now we iterate over Phi arguments, e.g. + // 2 args: VNF_Phi(SSA1, SSA2) + // 3 args: VNF_Phi(SSA1, VNF_Phi(SSA2, SSA3)) + // 4 args: VNF_Phi(SSA1, VNF_Phi(SSA2, VNF_Phi(SSA3, SSA4))) + // etc. + // + while (nextPhiArg != NoVN) + { + ValueNum current = nextPhiArg; + VNFuncApp phiArgFuncApp; + if (GetVNFunc(nextPhiArg, &phiArgFuncApp) && phiArgFuncApp.m_func == VNF_Phi) + { + current = phiArgFuncApp.m_args[0]; + nextPhiArg = phiArgFuncApp.m_args[1]; + } + else + { + // Ok this was the last Phi arg + nextPhiArg = NoVN; + } + + // Extract the real VN from the Phi arg + unsigned phiArgSsaNum = ConstantValue(current); + ValueNum phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(VNK_Conservative); + + // Now check the actual VN arg, if it's yet another PhiDef, walk it recursively + VNFuncApp argFuncApp; + if (GetVNFunc(phiArgVN, &argFuncApp) && (argFuncApp.m_func == VNF_PhiDef)) + { + if (maxDepth == 1) + { + JITDUMP("optWalkPhiVNs: maxDepth reached - bail out.\n"); + return false; + } + + // Call recursively. + // NOTE: it's possible for PhiDef to be cyclic, we don't try to detect them here and + // just rely on the maxDepth check to bail out (the max depth is usually small). + if (!VNWalkPhis(argFuncApp, walkPhiVnsFn, maxDepth - 1)) + { + return false; + } + } + else if (walkPhiVnsFn(m_pComp, phiArgVN) == PhiWalkResult::Abort) + { + // Caller realized that it doesn't make sense to continue any further + JITDUMP("optWalkPhiVNs: aborted by callback.\n"); + return false; + } + } + + return true; +} diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index b9592d8c8cae27..49fc61b89776a2 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -514,6 +514,14 @@ class ValueNumStore void PeelOffsets(ValueNum* vn, target_ssize_t* offset); + enum class PhiWalkResult + { + Continue, + Abort + }; + typedef PhiWalkResult(WalkPhiVnsFn)(Compiler* comp, ValueNum vn); + bool VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, int maxDepth = 2); + // And the single constant for an object reference type. static ValueNum VNForNull() { From 2e0563385349fbf1905e9ffc3ae87ba0c3167768 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 Jul 2024 17:40:05 +0200 Subject: [PATCH 02/10] Clean up + address some of the feedabck --- src/coreclr/jit/valuenum.cpp | 58 ++++++++++++++++++++++++------------ src/coreclr/jit/valuenum.h | 15 +++++++--- 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 5f12220f1c9671..0eb2d3f12d6bd5 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6499,15 +6499,16 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) case VNF_PhiDef: { // Inspect all PHI args (call IsVNNeverNegative for them) - return VNWalkPhis(funcApp, [](Compiler* comp, ValueNum phiVN) -> PhiWalkResult { + PhiDefWalkResult walkResult = VNWalkPhis(funcApp, [this](ValueNum phiVN) -> PhiArgWalkResult { // Bail out if the type is not integral - if (!varTypeIsIntegral(comp->vnStore->TypeOfVN(phiVN))) + if (!varTypeIsIntegral(TypeOfVN(phiVN))) { - return PhiWalkResult::Abort; + return PhiArgWalkResult::Abort; } - return comp->vnStore->IsVNNeverNegative(phiVN) ? PhiWalkResult::Continue : PhiWalkResult::Abort; + return IsVNNeverNegative(phiVN) ? PhiArgWalkResult::Continue : PhiArgWalkResult::Abort; }); + return walkResult == PhiDefWalkResult::Success; } default: @@ -15066,25 +15067,32 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) // VNWalkPhis: Walk the given PhiDef and call a callback on each Phi argument (its VN). // // Arguments: -// phiDefFunc - VNF_PhiDef function -// walkPhiVnsFn - The callback function to call on each Phi argument. -// maxDepth - Maximum depth to handle recursive PhiDef chains. +// phiDefFunc - VNF_PhiDef function +// phiArgVisitor - The callback function to call on each Phi argument. +// maxDepth - Maximum depth to handle recursive PhiDef chains. // // Return Value: // true if the walk was successful, false if the walk was aborted. // -bool ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, int maxDepth) +template +ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, + TPhiArgVisitorFunc phiArgVisitor, + int maxDepth) { assert(maxDepth > 0); assert(phiDefFunc.m_func == VNF_PhiDef); + // TODO: Consider enabling for VNF_PhiMemoryDef as well. // VNF_PhiDef(LclNum, SsaNum, VNF_Phi) unsigned lclNum = phiDefFunc.m_args[0]; ValueNum nextPhiArg = phiDefFunc.m_args[2]; - if ((lclNum == static_cast(-1)) || (nextPhiArg == NoVN)) + INDEBUG(int visited = 0); + + if ((lclNum == BAD_VAR_NUM) || (nextPhiArg == NoVN)) { - return false; + JITDUMP("We can't process this PhiDef - bail out.\n") + return PhiDefWalkResult::InvalidPhiDef; } // Now we iterate over Phi arguments, e.g. @@ -15097,8 +15105,11 @@ bool ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, { ValueNum current = nextPhiArg; VNFuncApp phiArgFuncApp; - if (GetVNFunc(nextPhiArg, &phiArgFuncApp) && phiArgFuncApp.m_func == VNF_Phi) + + // nextPhiArg is either VNF_Phi or SSA num (last) + if (GetVNFunc(nextPhiArg, &phiArgFuncApp)) { + assert(phiArgFuncApp.m_func == VNF_Phi); current = phiArgFuncApp.m_args[0]; nextPhiArg = phiArgFuncApp.m_args[1]; } @@ -15108,6 +15119,9 @@ bool ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, nextPhiArg = NoVN; } + // We expect a constant VN here representing the SSA number + assert(IsVNConstant(current)); + // Extract the real VN from the Phi arg unsigned phiArgSsaNum = ConstantValue(current); ValueNum phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(VNK_Conservative); @@ -15119,24 +15133,30 @@ bool ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, if (maxDepth == 1) { JITDUMP("optWalkPhiVNs: maxDepth reached - bail out.\n"); - return false; + return PhiDefWalkResult::HitLimitations; } // Call recursively. // NOTE: it's possible for PhiDef to be cyclic, we don't try to detect them here and // just rely on the maxDepth check to bail out (the max depth is usually small). - if (!VNWalkPhis(argFuncApp, walkPhiVnsFn, maxDepth - 1)) + PhiDefWalkResult result = VNWalkPhis(argFuncApp, phiArgVisitor, maxDepth - 1); + if (result != PhiDefWalkResult::Success) { - return false; + return result; } } - else if (walkPhiVnsFn(m_pComp, phiArgVN) == PhiWalkResult::Abort) + else { - // Caller realized that it doesn't make sense to continue any further - JITDUMP("optWalkPhiVNs: aborted by callback.\n"); - return false; + if (phiArgVisitor(phiArgVN) == PhiArgWalkResult::Abort) + { + // Caller realized that it doesn't make sense to continue any further + JITDUMP("optWalkPhiVNs: aborted by callback.\n"); + return PhiDefWalkResult::Aborted; + } + INDEBUG(visited++); } } - return true; + assert(visited > 0); + return PhiDefWalkResult::Success; } diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 49fc61b89776a2..19f4b2ce9af3f9 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -514,13 +514,20 @@ class ValueNumStore void PeelOffsets(ValueNum* vn, target_ssize_t* offset); - enum class PhiWalkResult + enum class PhiArgWalkResult { Continue, - Abort + Abort, }; - typedef PhiWalkResult(WalkPhiVnsFn)(Compiler* comp, ValueNum vn); - bool VNWalkPhis(VNFuncApp phiDefFunc, WalkPhiVnsFn walkPhiVnsFn, int maxDepth = 2); + enum class PhiDefWalkResult + { + Success, + Aborted, + InvalidPhiDef, + HitLimitations, + }; + template + PhiDefWalkResult VNWalkPhis(VNFuncApp phiDefFunc, TPhiArgVisitorFunc walkPhiVnsFn, int maxDepth = 2); // And the single constant for an object reference type. static ValueNum VNForNull() From aebf42d8c61cac0d71378972fcb40a81d0c4a99c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 Jul 2024 20:43:52 +0200 Subject: [PATCH 03/10] Address feedback --- src/coreclr/jit/valuenum.cpp | 100 ++++++++++++++++++++++++++--------- src/coreclr/jit/valuenum.h | 16 +++++- 2 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0eb2d3f12d6bd5..5a8fd5939a0274 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -3123,6 +3123,22 @@ class SmallValueNumSet } } + bool Lookup(ValueNum vn) + { + if (m_numElements <= ArrLen(m_inlineElements)) + { + for (unsigned i = 0; i < m_numElements; i++) + { + if (m_inlineElements[i] == vn) + { + return true; + } + } + return false; + } + return m_set->Lookup(vn); + } + void Add(Compiler* comp, ValueNum vn) { if (m_numElements <= ArrLen(m_inlineElements)) @@ -6497,9 +6513,10 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) #endif // FEATURE_HW_INTRINSICS case VNF_PhiDef: + case VNF_PhiMemoryDef: { // Inspect all PHI args (call IsVNNeverNegative for them) - PhiDefWalkResult walkResult = VNWalkPhis(funcApp, [this](ValueNum phiVN) -> PhiArgWalkResult { + PhiDefWalkResult walkResult = VNWalkPhis(vn, funcApp, [this](ValueNum phiVN) -> PhiArgWalkResult { // Bail out if the type is not integral if (!varTypeIsIntegral(TypeOfVN(phiVN))) { @@ -15064,37 +15081,55 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) } //-------------------------------------------------------------------------------- -// VNWalkPhis: Walk the given PhiDef and call a callback on each Phi argument (its VN). +// VNWalkPhis: Walk the given PhiDef/PhiMemoryDef and call a callback on each Phi argument (its VN). // // Arguments: -// phiDefFunc - VNF_PhiDef function +// phiDefVN - The VN of the PhiDef +// phiDefFunc - VNFuncApp for phiDefVN since caller typically has it anyway (to save TP). // phiArgVisitor - The callback function to call on each Phi argument. -// maxDepth - Maximum depth to handle recursive PhiDef chains. +// vnKind - The kind of VN to use (Conservative or Liberal) +// maxDepth - Maximum depth to handle recursive PhiDef chains. -1 means no limits. // // Return Value: // true if the walk was successful, false if the walk was aborted. // template -ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis(VNFuncApp phiDefFunc, - TPhiArgVisitorFunc phiArgVisitor, - int maxDepth) +ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis( + ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc phiArgVisitor, ValueNumKind vnKind, int maxDepth) { - assert(maxDepth > 0); - assert(phiDefFunc.m_func == VNF_PhiDef); - // TODO: Consider enabling for VNF_PhiMemoryDef as well. + SmallValueNumSet hashSet; + PhiDefWalkResult result = VNWalkPhisInternal(phiDefVN, phiDefFunc, phiArgVisitor, vnKind, maxDepth, hashSet); + return result; +} - // VNF_PhiDef(LclNum, SsaNum, VNF_Phi) - unsigned lclNum = phiDefFunc.m_args[0]; - ValueNum nextPhiArg = phiDefFunc.m_args[2]; +//-------------------------------------------------------------------------------- +// VNWalkPhisInternal: internal worker for VNWalkPhis, see comments in VNWalkPhis +// +template +ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhisInternal(ValueNum phiDefVN, + VNFuncApp phiDefFunc, + TPhiArgVisitorFunc phiArgVisitor, + ValueNumKind vnKind, + int maxDepth, + SmallValueNumSet& hashSet) +{ + assert((maxDepth == -1) || (maxDepth > 0)); + assert((phiDefFunc.m_func == VNF_PhiDef) || (phiDefFunc.m_func == VNF_PhiMemoryDef)); - INDEBUG(int visited = 0); + const bool isMemory = (phiDefFunc.m_func == VNF_PhiMemoryDef); - if ((lclNum == BAD_VAR_NUM) || (nextPhiArg == NoVN)) + // VNF_PhiDef(LclNum, SsaNum, VNF_Phi) + // VNF_PhiMemoryDef(BB, VNF_Phi) + ValueNum nextPhiArg = isMemory ? phiDefFunc.m_args[1] : phiDefFunc.m_args[2]; + if ((nextPhiArg == NoVN) || (!isMemory && (phiDefFunc.m_args[0] == BAD_VAR_NUM))) { - JITDUMP("We can't process this PhiDef - bail out.\n") + JITDUMP("We can't process this PhiDef/PhiMemoryDef - bail out.\n"); return PhiDefWalkResult::InvalidPhiDef; } + assert(!hashSet.Lookup(phiDefVN)); + hashSet.Add(m_pComp, phiDefVN); + // Now we iterate over Phi arguments, e.g. // 2 args: VNF_Phi(SSA1, SSA2) // 3 args: VNF_Phi(SSA1, VNF_Phi(SSA2, SSA3)) @@ -15124,11 +15159,29 @@ ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis(VNFuncApp phi // Extract the real VN from the Phi arg unsigned phiArgSsaNum = ConstantValue(current); - ValueNum phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(VNK_Conservative); + + ValueNum phiArgVN; + if (isMemory) + { + phiArgVN = m_pComp->GetMemoryPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnKind); + } + else + { + // NOTE: it's unfortunate that LclNum is not a VN. + unsigned phiDefLclNum = phiDefFunc.m_args[0]; + phiArgVN = m_pComp->lvaTable[phiDefLclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnKind); + } + + if (hashSet.Lookup(phiArgVN)) + { + // We already visited this VN which means we have a cycle + continue; + } // Now check the actual VN arg, if it's yet another PhiDef, walk it recursively VNFuncApp argFuncApp; - if (GetVNFunc(phiArgVN, &argFuncApp) && (argFuncApp.m_func == VNF_PhiDef)) + if (GetVNFunc(phiArgVN, &argFuncApp) && + ((argFuncApp.m_func == VNF_PhiDef) || (argFuncApp.m_func == VNF_PhiMemoryDef))) { if (maxDepth == 1) { @@ -15136,10 +15189,10 @@ ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis(VNFuncApp phi return PhiDefWalkResult::HitLimitations; } - // Call recursively. - // NOTE: it's possible for PhiDef to be cyclic, we don't try to detect them here and - // just rely on the maxDepth check to bail out (the max depth is usually small). - PhiDefWalkResult result = VNWalkPhis(argFuncApp, phiArgVisitor, maxDepth - 1); + // MaxDepth being -1 means we don't have depth limits + int newDepth = maxDepth == -1 ? -1 : maxDepth - 1; + PhiDefWalkResult result = + VNWalkPhisInternal(phiArgVN, argFuncApp, phiArgVisitor, vnKind, newDepth, hashSet); if (result != PhiDefWalkResult::Success) { return result; @@ -15153,10 +15206,9 @@ ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis(VNFuncApp phi JITDUMP("optWalkPhiVNs: aborted by callback.\n"); return PhiDefWalkResult::Aborted; } - INDEBUG(visited++); + hashSet.Add(m_pComp, phiArgVN); } } - assert(visited > 0); return PhiDefWalkResult::Success; } diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 19f4b2ce9af3f9..86b8315e17c614 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -527,7 +527,21 @@ class ValueNumStore HitLimitations, }; template - PhiDefWalkResult VNWalkPhis(VNFuncApp phiDefFunc, TPhiArgVisitorFunc walkPhiVnsFn, int maxDepth = 2); + PhiDefWalkResult VNWalkPhis(ValueNum phiDefVN, + VNFuncApp phiDefFunc, + TPhiArgVisitorFunc walkPhiVnsFn, + ValueNumKind vnKind = VNK_Conservative, + int maxDepth = -1); + +private: + template + PhiDefWalkResult VNWalkPhisInternal(ValueNum phiDefVN, + VNFuncApp phiDefFunc, + TPhiArgVisitorFunc walkPhiVnsFn, + ValueNumKind vnKind, + int maxDepth, + class SmallValueNumSet& hashSet); +public: // And the single constant for an object reference type. static ValueNum VNForNull() From d8bf5cc6fbd7bdda2aa4d0f39d8dfbd6c78c5805 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 Jul 2024 22:25:27 +0200 Subject: [PATCH 04/10] Clean up --- src/coreclr/jit/valuenum.cpp | 54 ++++++++++++++++++++++++++++-------- src/coreclr/jit/valuenum.h | 9 +++--- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 5a8fd5939a0274..182415a5ff830f 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6516,7 +6516,9 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) case VNF_PhiMemoryDef: { // Inspect all PHI args (call IsVNNeverNegative for them) - PhiDefWalkResult walkResult = VNWalkPhis(vn, funcApp, [this](ValueNum phiVN) -> PhiArgWalkResult { + PhiDefWalkResult walkResult = VNWalkPhis( + vn, funcApp, + [this](ValueNum phiVN) -> PhiArgWalkResult { // Bail out if the type is not integral if (!varTypeIsIntegral(TypeOfVN(phiVN))) { @@ -6524,8 +6526,9 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) } return IsVNNeverNegative(phiVN) ? PhiArgWalkResult::Continue : PhiArgWalkResult::Abort; - }); - return walkResult == PhiDefWalkResult::Success; + }, + /* maxDepth */ 2); + return walkResult == PhiDefWalkResult::Completed; } default: @@ -15041,6 +15044,24 @@ CORINFO_CLASS_HANDLE ValueNumStore::GetObjectType(ValueNum vn, bool* pIsExact, b return m_pComp->info.compCompHnd->getBuiltinClass(CLASSID_RUNTIME_TYPE); } + if (func == VNF_PhiDef || func == VNF_PhiMemoryDef) + { + // Inspect all PHI args (call IsVNNeverNegative for them) + PhiDefWalkResult walkResult = VNWalkPhis( + vn, funcApp, + [this](ValueNum phiVN) -> PhiArgWalkResult { + // Bail out if the type is not integral + if (!varTypeIsIntegral(TypeOfVN(phiVN))) + { + return PhiArgWalkResult::Abort; + } + + return IsVNNeverNegative(phiVN) ? PhiArgWalkResult::Continue : PhiArgWalkResult::Abort; + }, + /* maxDepth */ 2); + return walkResult == PhiDefWalkResult::Completed; + } + return NO_CLASS_HANDLE; } @@ -15087,18 +15108,25 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) // phiDefVN - The VN of the PhiDef // phiDefFunc - VNFuncApp for phiDefVN since caller typically has it anyway (to save TP). // phiArgVisitor - The callback function to call on each Phi argument. -// vnKind - The kind of VN to use (Conservative or Liberal) // maxDepth - Maximum depth to handle recursive PhiDef chains. -1 means no limits. +// vnKind - The kind of VN to use (Conservative or Liberal) // // Return Value: // true if the walk was successful, false if the walk was aborted. // template ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis( - ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc phiArgVisitor, ValueNumKind vnKind, int maxDepth) + ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc phiArgVisitor, int maxDepth, ValueNumKind vnKind) { + int realVNsVisited = 0; SmallValueNumSet hashSet; - PhiDefWalkResult result = VNWalkPhisInternal(phiDefVN, phiDefFunc, phiArgVisitor, vnKind, maxDepth, hashSet); + PhiDefWalkResult result = + VNWalkPhisInternal(phiDefVN, phiDefFunc, phiArgVisitor, maxDepth, vnKind, realVNsVisited, hashSet); + if ((result == PhiDefWalkResult::Completed) && (realVNsVisited == 0)) + { + // We somehow didn't visit any real VN - bail out. + return PhiDefWalkResult::InvalidPhiDef; + } return result; } @@ -15109,8 +15137,9 @@ template ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhisInternal(ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc phiArgVisitor, - ValueNumKind vnKind, int maxDepth, + ValueNumKind vnKind, + int& realVNsVisited, SmallValueNumSet& hashSet) { assert((maxDepth == -1) || (maxDepth > 0)); @@ -15121,9 +15150,9 @@ ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhisInternal(ValueNum // VNF_PhiDef(LclNum, SsaNum, VNF_Phi) // VNF_PhiMemoryDef(BB, VNF_Phi) ValueNum nextPhiArg = isMemory ? phiDefFunc.m_args[1] : phiDefFunc.m_args[2]; - if ((nextPhiArg == NoVN) || (!isMemory && (phiDefFunc.m_args[0] == BAD_VAR_NUM))) + if (!isMemory && (phiDefFunc.m_args[0] == BAD_VAR_NUM)) { - JITDUMP("We can't process this PhiDef/PhiMemoryDef - bail out.\n"); + JITDUMP("We can't process this PhiDef - bail out.\n"); return PhiDefWalkResult::InvalidPhiDef; } @@ -15192,8 +15221,8 @@ ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhisInternal(ValueNum // MaxDepth being -1 means we don't have depth limits int newDepth = maxDepth == -1 ? -1 : maxDepth - 1; PhiDefWalkResult result = - VNWalkPhisInternal(phiArgVN, argFuncApp, phiArgVisitor, vnKind, newDepth, hashSet); - if (result != PhiDefWalkResult::Success) + VNWalkPhisInternal(phiArgVN, argFuncApp, phiArgVisitor, newDepth, vnKind, realVNsVisited, hashSet); + if (result != PhiDefWalkResult::Completed) { return result; } @@ -15206,9 +15235,10 @@ ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhisInternal(ValueNum JITDUMP("optWalkPhiVNs: aborted by callback.\n"); return PhiDefWalkResult::Aborted; } + realVNsVisited++; hashSet.Add(m_pComp, phiArgVN); } } - return PhiDefWalkResult::Success; + return PhiDefWalkResult::Completed; } diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 86b8315e17c614..7bd4c1d5dbba8c 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -521,7 +521,7 @@ class ValueNumStore }; enum class PhiDefWalkResult { - Success, + Completed, Aborted, InvalidPhiDef, HitLimitations, @@ -530,16 +530,17 @@ class ValueNumStore PhiDefWalkResult VNWalkPhis(ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc walkPhiVnsFn, - ValueNumKind vnKind = VNK_Conservative, - int maxDepth = -1); + int maxDepth = -1, + ValueNumKind vnKind = VNK_Conservative); private: template PhiDefWalkResult VNWalkPhisInternal(ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc walkPhiVnsFn, - ValueNumKind vnKind, int maxDepth, + ValueNumKind vnKind, + int& realVNsVisited, class SmallValueNumSet& hashSet); public: From 54e2f7a52a3ec71e00ecbc2f22b3f9f7ada71901 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 Jul 2024 22:27:33 +0200 Subject: [PATCH 05/10] remove unrelated change --- src/coreclr/jit/valuenum.cpp | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 182415a5ff830f..a82ab87b7ed50f 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -15044,24 +15044,6 @@ CORINFO_CLASS_HANDLE ValueNumStore::GetObjectType(ValueNum vn, bool* pIsExact, b return m_pComp->info.compCompHnd->getBuiltinClass(CLASSID_RUNTIME_TYPE); } - if (func == VNF_PhiDef || func == VNF_PhiMemoryDef) - { - // Inspect all PHI args (call IsVNNeverNegative for them) - PhiDefWalkResult walkResult = VNWalkPhis( - vn, funcApp, - [this](ValueNum phiVN) -> PhiArgWalkResult { - // Bail out if the type is not integral - if (!varTypeIsIntegral(TypeOfVN(phiVN))) - { - return PhiArgWalkResult::Abort; - } - - return IsVNNeverNegative(phiVN) ? PhiArgWalkResult::Continue : PhiArgWalkResult::Abort; - }, - /* maxDepth */ 2); - return walkResult == PhiDefWalkResult::Completed; - } - return NO_CLASS_HANDLE; } From b9909ce30fd0f8b48401aae97f98db0fc760b677 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 Jul 2024 22:40:45 +0200 Subject: [PATCH 06/10] add more comments --- src/coreclr/jit/valuenum.cpp | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index a82ab87b7ed50f..176379b73e6285 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6515,20 +6515,19 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) case VNF_PhiDef: case VNF_PhiMemoryDef: { - // Inspect all PHI args (call IsVNNeverNegative for them) - PhiDefWalkResult walkResult = VNWalkPhis( - vn, funcApp, - [this](ValueNum phiVN) -> PhiArgWalkResult { + // Inspect all PHI args - if all of them are non-negative, + // then the PHI itself is non-negative too (even if it has cycles). + auto phiVisitor = [this](ValueNum phiVN) -> PhiArgWalkResult { // Bail out if the type is not integral if (!varTypeIsIntegral(TypeOfVN(phiVN))) { return PhiArgWalkResult::Abort; } - return IsVNNeverNegative(phiVN) ? PhiArgWalkResult::Continue : PhiArgWalkResult::Abort; - }, - /* maxDepth */ 2); - return walkResult == PhiDefWalkResult::Completed; + }; + + // maxDepth is set to 2 to improve JIT TP + return VNWalkPhis(vn, funcApp, phiVisitor, /* maxDepth */ 2) == PhiDefWalkResult::Completed; } default: @@ -15084,7 +15083,7 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) } //-------------------------------------------------------------------------------- -// VNWalkPhis: Walk the given PhiDef/PhiMemoryDef and call a callback on each Phi argument (its VN). +// VNWalkPhis: Walk the given PhiDef/PhiMemoryDef and call phiArgVisitor on each real VN. // // Arguments: // phiDefVN - The VN of the PhiDef @@ -15094,7 +15093,11 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) // vnKind - The kind of VN to use (Conservative or Liberal) // // Return Value: -// true if the walk was successful, false if the walk was aborted. +// PhiDefWalkResult::InvalidPhiDef if any PhiDef didn't allow us to make any assumptions +// PhiDefWalkResult::HitLimitations if we hit the maxDepth limit (for TP reasons) +// PhiDefWalkResult::Aborted if any phiArgVisitor returned PhiArgWalkResult::Abort +// PhiDefWalkResult::Completed if the walk was successful (all phiArgVisitors returned +// PhiArgWalkResult::Continue) and the phiArgVisitor was called at least once // template ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis( From 9a2da3608cd1954d515e36d5a703618841dbaea9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 24 Jul 2024 01:55:52 +0200 Subject: [PATCH 07/10] Refactoring --- src/coreclr/jit/valuenum.cpp | 308 +++++++++++++++-------------------- src/coreclr/jit/valuenum.h | 29 ++-- 2 files changed, 138 insertions(+), 199 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 44070808c95066..4c51f17ea16238 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -3242,6 +3242,7 @@ class SmallValueNumSet bool Lookup(ValueNum vn) { + // O(N) lookup for inline elements if (m_numElements <= ArrLen(m_inlineElements)) { for (unsigned i = 0; i < m_numElements; i++) @@ -3251,12 +3252,16 @@ class SmallValueNumSet return true; } } + + // Not found return false; } + return m_set->Lookup(vn); } - void Add(Compiler* comp, ValueNum vn) + // Returns false if the value already exists + bool Add(Compiler* comp, ValueNum vn) { if (m_numElements <= ArrLen(m_inlineElements)) { @@ -3264,7 +3269,8 @@ class SmallValueNumSet { if (m_inlineElements[i] == vn) { - return; + // Already exists + return false; } } @@ -3287,12 +3293,12 @@ class SmallValueNumSet m_numElements++; assert(m_numElements == set->GetCount()); } + return true; } - else - { - m_set->Set(vn, true, ValueNumSet::SetKind::Overwrite); - m_numElements = m_set->GetCount(); - } + + bool exists = m_set->Set(vn, true, ValueNumSet::SetKind::Overwrite); + m_numElements = m_set->GetCount(); + return !exists; } }; @@ -6542,86 +6548,72 @@ bool ValueNumStore::IsVNInt32Constant(ValueNum vn) bool ValueNumStore::IsVNNeverNegative(ValueNum vn) { - assert(varTypeIsIntegral(TypeOfVN(vn))); - - if (IsVNConstant(vn)) - { - var_types vnTy = TypeOfVN(vn); - if (vnTy == TYP_INT) + auto unwrapper = [this](ValueNum vn) -> Unwrap { + if ((vn == NoVN) || !varTypeIsIntegral(TypeOfVN(vn))) { - return GetConstantInt32(vn) >= 0; + return Unwrap::Abort; } - else if (vnTy == TYP_LONG) + + if (IsVNConstant(vn)) { - return GetConstantInt64(vn) >= 0; + var_types vnTy = TypeOfVN(vn); + if (vnTy == TYP_INT) + { + return GetConstantInt32(vn) >= 0 ? Unwrap::Continue : Unwrap::Abort; + } + if (vnTy == TYP_LONG) + { + return GetConstantInt64(vn) >= 0 ? Unwrap::Continue : Unwrap::Abort; + } + return Unwrap::Abort; } - return false; - } - - // Array length can never be negative. - if (IsVNArrLen(vn)) - { - return true; - } + // Array length can never be negative. + if (IsVNArrLen(vn)) + { + return Unwrap::Continue; + } - VNFuncApp funcApp; - if (GetVNFunc(vn, &funcApp)) - { - switch (funcApp.m_func) + VNFuncApp funcApp; + if (GetVNFunc(vn, &funcApp)) { - case VNF_GE_UN: - case VNF_GT_UN: - case VNF_LE_UN: - case VNF_LT_UN: - case VNF_COUNT: - case VNF_ADD_UN_OVF: - case VNF_SUB_UN_OVF: - case VNF_MUL_UN_OVF: + switch (funcApp.m_func) + { + case VNF_GE_UN: + case VNF_GT_UN: + case VNF_LE_UN: + case VNF_LT_UN: + case VNF_COUNT: + case VNF_ADD_UN_OVF: + case VNF_SUB_UN_OVF: + case VNF_MUL_UN_OVF: #ifdef FEATURE_HW_INTRINSICS #ifdef TARGET_XARCH - case VNF_HWI_POPCNT_PopCount: - case VNF_HWI_POPCNT_X64_PopCount: - case VNF_HWI_LZCNT_LeadingZeroCount: - case VNF_HWI_LZCNT_X64_LeadingZeroCount: - case VNF_HWI_BMI1_TrailingZeroCount: - case VNF_HWI_BMI1_X64_TrailingZeroCount: - return true; + case VNF_HWI_POPCNT_PopCount: + case VNF_HWI_POPCNT_X64_PopCount: + case VNF_HWI_LZCNT_LeadingZeroCount: + case VNF_HWI_LZCNT_X64_LeadingZeroCount: + case VNF_HWI_BMI1_TrailingZeroCount: + case VNF_HWI_BMI1_X64_TrailingZeroCount: + return Unwrap::Continue; #elif defined(TARGET_ARM64) - case VNF_HWI_AdvSimd_PopCount: - case VNF_HWI_AdvSimd_LeadingZeroCount: - case VNF_HWI_AdvSimd_LeadingSignCount: - case VNF_HWI_ArmBase_LeadingZeroCount: - case VNF_HWI_ArmBase_Arm64_LeadingZeroCount: - case VNF_HWI_ArmBase_Arm64_LeadingSignCount: - return true; + case VNF_HWI_AdvSimd_PopCount: + case VNF_HWI_AdvSimd_LeadingZeroCount: + case VNF_HWI_AdvSimd_LeadingSignCount: + case VNF_HWI_ArmBase_LeadingZeroCount: + case VNF_HWI_ArmBase_Arm64_LeadingZeroCount: + case VNF_HWI_ArmBase_Arm64_LeadingSignCount: + return Unwrap::Continue; #endif #endif // FEATURE_HW_INTRINSICS - case VNF_PhiDef: - case VNF_PhiMemoryDef: - { - // Inspect all PHI args - if all of them are non-negative, - // then the PHI itself is non-negative too (even if it has cycles). - auto phiVisitor = [this](ValueNum phiVN) -> PhiArgWalkResult { - // Bail out if the type is not integral - if (!varTypeIsIntegral(TypeOfVN(phiVN))) - { - return PhiArgWalkResult::Abort; - } - return IsVNNeverNegative(phiVN) ? PhiArgWalkResult::Continue : PhiArgWalkResult::Abort; - }; - - // maxDepth is set to 2 to improve JIT TP - return VNWalkPhis(vn, funcApp, phiVisitor, /* maxDepth */ 2) == PhiDefWalkResult::Completed; + default: + break; } - - default: - break; } - } - - return false; + return Unwrap::Abort; + }; + return VNUnwrapPhis(vn, unwrapper, /* maxDepth */ 2) == UnwrapResult::Completed; } GenTreeFlags ValueNumStore::GetHandleFlags(ValueNum vn) @@ -15200,147 +15192,101 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) } //-------------------------------------------------------------------------------- -// VNWalkPhis: Walk the given PhiDef/PhiMemoryDef and call phiArgVisitor on each real VN. +// VNUnwrapPhis: given a VN, call the specified callback function on it. In case if the VN +// is a phi node, recursively call the callback function on all the VN arguments of that phi node. // // Arguments: -// phiDefVN - The VN of the PhiDef -// phiDefFunc - VNFuncApp for phiDefVN since caller typically has it anyway (to save TP). -// phiArgVisitor - The callback function to call on each Phi argument. -// maxDepth - Maximum depth to handle recursive PhiDef chains. -1 means no limits. -// vnKind - The kind of VN to use (Conservative or Liberal) +// vn - The VN to unwrap +// argVisitor - The callback function to call on the vn and its PHI arguments if any +// maxDepth - Maximum depth to handle recursive PHI chains. -1 means no limits. +// vnk - The kind of VN to use (Conservative or Liberal) // // Return Value: -// PhiDefWalkResult::InvalidPhiDef if any PhiDef didn't allow us to make any assumptions -// PhiDefWalkResult::HitLimitations if we hit the maxDepth limit (for TP reasons) -// PhiDefWalkResult::Aborted if any phiArgVisitor returned PhiArgWalkResult::Abort -// PhiDefWalkResult::Completed if the walk was successful (all phiArgVisitors returned -// PhiArgWalkResult::Continue) and the phiArgVisitor was called at least once +// UnwrapResult::MaxDepthReached - we hit the maxDepth limit (for TP reasons) +// UnwrapResult::Aborted - any argVisitor returned Unwrap::Abort +// UnwrapResult::Completed - unwrap was successful (all argVisitor returned Unwrap::Continue) // -template -ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhis( - ValueNum phiDefVN, VNFuncApp phiDefFunc, TPhiArgVisitorFunc phiArgVisitor, int maxDepth, ValueNumKind vnKind) +template +ValueNumStore::UnwrapResult ValueNumStore::VNUnwrapPhis(ValueNum vn, + TArgVisitor argVisitor, + int maxDepth, + ValueNumKind vnk) { - int realVNsVisited = 0; SmallValueNumSet hashSet; - PhiDefWalkResult result = - VNWalkPhisInternal(phiDefVN, phiDefFunc, phiArgVisitor, maxDepth, vnKind, realVNsVisited, hashSet); - if ((result == PhiDefWalkResult::Completed) && (realVNsVisited == 0)) - { - // We somehow didn't visit any real VN - bail out. - return PhiDefWalkResult::InvalidPhiDef; - } + UnwrapResult result = VNUnwrapPhisInternal(vn, argVisitor, maxDepth, vnk, hashSet); return result; } //-------------------------------------------------------------------------------- -// VNWalkPhisInternal: internal worker for VNWalkPhis, see comments in VNWalkPhis +// VNUnwrapPhisInternal: internal worker for VNUnwrapPhis, see comments above. // -template -ValueNumStore::PhiDefWalkResult ValueNumStore::VNWalkPhisInternal(ValueNum phiDefVN, - VNFuncApp phiDefFunc, - TPhiArgVisitorFunc phiArgVisitor, - int maxDepth, - ValueNumKind vnKind, - int& realVNsVisited, - SmallValueNumSet& hashSet) +template +ValueNumStore::UnwrapResult ValueNumStore::VNUnwrapPhisInternal( + ValueNum vn, TArgVisitor argVisitor, int maxDepth, ValueNumKind vnk, SmallValueNumSet& hashSet) { - assert((maxDepth == -1) || (maxDepth > 0)); - assert((phiDefFunc.m_func == VNF_PhiDef) || (phiDefFunc.m_func == VNF_PhiMemoryDef)); - - const bool isMemory = (phiDefFunc.m_func == VNF_PhiMemoryDef); + assert(maxDepth >= -1); - // VNF_PhiDef(LclNum, SsaNum, VNF_Phi) - // VNF_PhiMemoryDef(BB, VNF_Phi) - ValueNum nextPhiArg = isMemory ? phiDefFunc.m_args[1] : phiDefFunc.m_args[2]; - if (!isMemory && (phiDefFunc.m_args[0] == BAD_VAR_NUM)) + if (!hashSet.Add(m_pComp, vn)) { - JITDUMP("We can't process this PhiDef - bail out.\n"); - return PhiDefWalkResult::InvalidPhiDef; + // Recursive PhiDef chain detected + return UnwrapResult::Completed; } - assert(!hashSet.Lookup(phiDefVN)); - hashSet.Add(m_pComp, phiDefVN); + unsigned* ssaArgs; + unsigned numSsaArgs; + unsigned lclNum; - // Now we iterate over Phi arguments, e.g. - // 2 args: VNF_Phi(SSA1, SSA2) - // 3 args: VNF_Phi(SSA1, VNF_Phi(SSA2, SSA3)) - // 4 args: VNF_Phi(SSA1, VNF_Phi(SSA2, VNF_Phi(SSA3, SSA4))) - // etc. - // - while (nextPhiArg != NoVN) + VNPhiDef phiDef; + VNMemoryPhiDef memoryPhiDef; + if (GetPhiDef(vn, &phiDef)) + { + lclNum = phiDef.LclNum; + ssaArgs = phiDef.SsaArgs; + numSsaArgs = phiDef.NumArgs; + } + else if (GetMemoryPhiDef(vn, &memoryPhiDef)) + { + lclNum = BAD_VAR_NUM; + ssaArgs = memoryPhiDef.SsaArgs; + numSsaArgs = memoryPhiDef.NumArgs; + } + else { - ValueNum current = nextPhiArg; - VNFuncApp phiArgFuncApp; + // Not a phi node - handle normally and return + return argVisitor(vn) == Unwrap::Abort ? UnwrapResult::Aborted : UnwrapResult::Completed; + } - // nextPhiArg is either VNF_Phi or SSA num (last) - if (GetVNFunc(nextPhiArg, &phiArgFuncApp)) - { - assert(phiArgFuncApp.m_func == VNF_Phi); - current = phiArgFuncApp.m_args[0]; - nextPhiArg = phiArgFuncApp.m_args[1]; - } - else - { - // Ok this was the last Phi arg - nextPhiArg = NoVN; - } + // At this point vn is a PHI definition node - // We expect a constant VN here representing the SSA number - assert(IsVNConstant(current)); + if (maxDepth == 0) + { + return UnwrapResult::MaxDepthReached; + } - // Extract the real VN from the Phi arg - unsigned phiArgSsaNum = ConstantValue(current); + assert(ssaArgs != nullptr); + assert(numSsaArgs > 0); + for (unsigned i = 0; i < numSsaArgs; i++) + { ValueNum phiArgVN; - if (isMemory) + if (lclNum != BAD_VAR_NUM) { - phiArgVN = m_pComp->GetMemoryPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnKind); + phiArgVN = m_pComp->lvaGetDesc(lclNum)->GetPerSsaData(ssaArgs[i])->m_vnPair.Get(vnk); } else { - // NOTE: it's unfortunate that LclNum is not a VN. - unsigned phiDefLclNum = phiDefFunc.m_args[0]; - phiArgVN = m_pComp->lvaTable[phiDefLclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnKind); + phiArgVN = m_pComp->GetMemoryPerSsaData(ssaArgs[i])->m_vnPair.Get(vnk); } - if (hashSet.Lookup(phiArgVN)) - { - // We already visited this VN which means we have a cycle - continue; - } + // maxDepth being -1 means we don't have depth limits + int newDepth = maxDepth == -1 ? -1 : maxDepth - 1; - // Now check the actual VN arg, if it's yet another PhiDef, walk it recursively - VNFuncApp argFuncApp; - if (GetVNFunc(phiArgVN, &argFuncApp) && - ((argFuncApp.m_func == VNF_PhiDef) || (argFuncApp.m_func == VNF_PhiMemoryDef))) + // TODO-VN: Consider getting rid of this recursion and using an ArrayStack instead. + UnwrapResult result = VNUnwrapPhisInternal(phiArgVN, argVisitor, newDepth, vnk, hashSet); + if (result != UnwrapResult::Completed) { - if (maxDepth == 1) - { - JITDUMP("optWalkPhiVNs: maxDepth reached - bail out.\n"); - return PhiDefWalkResult::HitLimitations; - } - - // MaxDepth being -1 means we don't have depth limits - int newDepth = maxDepth == -1 ? -1 : maxDepth - 1; - PhiDefWalkResult result = - VNWalkPhisInternal(phiArgVN, argFuncApp, phiArgVisitor, newDepth, vnKind, realVNsVisited, hashSet); - if (result != PhiDefWalkResult::Completed) - { - return result; - } - } - else - { - if (phiArgVisitor(phiArgVN) == PhiArgWalkResult::Abort) - { - // Caller realized that it doesn't make sense to continue any further - JITDUMP("optWalkPhiVNs: aborted by callback.\n"); - return PhiDefWalkResult::Aborted; - } - realVNsVisited++; - hashSet.Add(m_pComp, phiArgVN); + return result; } } - - return PhiDefWalkResult::Completed; + return UnwrapResult::Completed; } diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 3f8172c6d0ba2b..9e4f472891c2b0 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -529,34 +529,27 @@ class ValueNumStore void PeelOffsets(ValueNum* vn, target_ssize_t* offset); - enum class PhiArgWalkResult + enum class Unwrap { Continue, Abort, }; - enum class PhiDefWalkResult + enum class UnwrapResult { Completed, Aborted, - InvalidPhiDef, - HitLimitations, + MaxDepthReached, }; - template - PhiDefWalkResult VNWalkPhis(ValueNum phiDefVN, - VNFuncApp phiDefFunc, - TPhiArgVisitorFunc walkPhiVnsFn, - int maxDepth = -1, - ValueNumKind vnKind = VNK_Conservative); + template + UnwrapResult VNUnwrapPhis(ValueNum vn, + TArgVisitor argVisitor, + int maxDepth = -1, + ValueNumKind vnk = VNK_Conservative); private: - template - PhiDefWalkResult VNWalkPhisInternal(ValueNum phiDefVN, - VNFuncApp phiDefFunc, - TPhiArgVisitorFunc walkPhiVnsFn, - int maxDepth, - ValueNumKind vnKind, - int& realVNsVisited, - class SmallValueNumSet& hashSet); + template + UnwrapResult VNUnwrapPhisInternal( + ValueNum vn, TArgVisitor argVisitor, int maxDepth, ValueNumKind vnk, class SmallValueNumSet& hashSet); public: // And the single constant for an object reference type. From 5aa7cad25ef63453152cda4efcb92cb887f34902 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 24 Jul 2024 20:50:50 +0200 Subject: [PATCH 08/10] Address feedback --- src/coreclr/jit/valuenum.cpp | 250 +++++++++-------------------------- src/coreclr/jit/valuenum.h | 115 ++++++++++++++-- 2 files changed, 168 insertions(+), 197 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 4c51f17ea16238..c162d108c9b4e5 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -3204,103 +3204,81 @@ ValueNum ValueNumStore::VNForMapPhysicalSelect( return result; } -typedef JitHashTable, bool> ValueNumSet; - -class SmallValueNumSet +bool ValueNumStore::SmallValueNumSet::Lookup(ValueNum vn) { - union + // O(N) lookup for inline elements + if (m_numElements <= ArrLen(m_inlineElements)) { - ValueNum m_inlineElements[4]; - ValueNumSet* m_set; - }; - unsigned m_numElements = 0; - -public: - unsigned Count() - { - return m_numElements; - } - - template - void ForEach(Func func) - { - if (m_numElements <= ArrLen(m_inlineElements)) - { - for (unsigned i = 0; i < m_numElements; i++) - { - func(m_inlineElements[i]); - } - } - else + for (unsigned i = 0; i < m_numElements; i++) { - for (ValueNum vn : ValueNumSet::KeyIteration(m_set)) + if (m_inlineElements[i] == vn) { - func(vn); + return true; } } + + // Not found + return false; } - bool Lookup(ValueNum vn) + return m_set->Lookup(vn); +} + +// Returns false if the value already exists +bool ValueNumStore::SmallValueNumSet::Add(Compiler* comp, ValueNum vn) +{ + if (m_numElements <= ArrLen(m_inlineElements)) { - // O(N) lookup for inline elements - if (m_numElements <= ArrLen(m_inlineElements)) + for (unsigned i = 0; i < m_numElements; i++) { - for (unsigned i = 0; i < m_numElements; i++) + if (m_inlineElements[i] == vn) { - if (m_inlineElements[i] == vn) - { - return true; - } + // Already exists + return false; } - - // Not found - return false; } - return m_set->Lookup(vn); - } - - // Returns false if the value already exists - bool Add(Compiler* comp, ValueNum vn) - { - if (m_numElements <= ArrLen(m_inlineElements)) + if (m_numElements < ArrLen(m_inlineElements)) { - for (unsigned i = 0; i < m_numElements; i++) + m_inlineElements[m_numElements] = vn; + m_numElements++; + } + else + { + ValueNumSet* set = new (comp, CMK_ValueNumber) ValueNumSet(comp->getAllocator(CMK_ValueNumber)); + for (ValueNum oldVn : m_inlineElements) { - if (m_inlineElements[i] == vn) - { - // Already exists - return false; - } + set->Set(oldVn, true); } - if (m_numElements < ArrLen(m_inlineElements)) - { - m_inlineElements[m_numElements] = vn; - m_numElements++; - } - else - { - ValueNumSet* set = new (comp, CMK_ValueNumber) ValueNumSet(comp->getAllocator(CMK_ValueNumber)); - for (ValueNum oldVn : m_inlineElements) - { - set->Set(oldVn, true); - } - - set->Set(vn, true); + set->Set(vn, true); - m_set = set; - m_numElements++; - assert(m_numElements == set->GetCount()); - } - return true; + m_set = set; + m_numElements++; + assert(m_numElements == set->GetCount()); } - - bool exists = m_set->Set(vn, true, ValueNumSet::SetKind::Overwrite); - m_numElements = m_set->GetCount(); - return !exists; + return true; } -}; + + bool exists = m_set->Set(vn, true, ValueNumSet::SetKind::Overwrite); + m_numElements = m_set->GetCount(); + return !exists; +} + +//------------------------------------------------------------------------------ +// VNPhiDefToVN: Extracts the VN for a specific argument of a phi definition. +// +// Arguments: +// phiDef - The phi definition +// ssaArgNum - The argument number to extract +// +// Return Value: +// The VN for the specified argument of the phi definition. +// +ValueNum ValueNumStore::VNPhiDefToVN(const VNPhiDef& phiDef, unsigned ssaArgNum) +{ + return m_pComp->lvaGetDesc(phiDef.LclNum)->GetPerSsaData(phiDef.SsaArgs[ssaArgNum])->m_vnPair.Get(VNK_Conservative); +} //------------------------------------------------------------------------------ // VNForMapSelectInner: Select value from a map and record loop memory dependencies. @@ -6548,10 +6526,10 @@ bool ValueNumStore::IsVNInt32Constant(ValueNum vn) bool ValueNumStore::IsVNNeverNegative(ValueNum vn) { - auto unwrapper = [this](ValueNum vn) -> Unwrap { + auto unwrapper = [this](ValueNum vn) -> VNVisit { if ((vn == NoVN) || !varTypeIsIntegral(TypeOfVN(vn))) { - return Unwrap::Abort; + return VNVisit::Abort; } if (IsVNConstant(vn)) @@ -6559,19 +6537,19 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) var_types vnTy = TypeOfVN(vn); if (vnTy == TYP_INT) { - return GetConstantInt32(vn) >= 0 ? Unwrap::Continue : Unwrap::Abort; + return GetConstantInt32(vn) >= 0 ? VNVisit::Continue : VNVisit::Abort; } if (vnTy == TYP_LONG) { - return GetConstantInt64(vn) >= 0 ? Unwrap::Continue : Unwrap::Abort; + return GetConstantInt64(vn) >= 0 ? VNVisit::Continue : VNVisit::Abort; } - return Unwrap::Abort; + return VNVisit::Abort; } // Array length can never be negative. if (IsVNArrLen(vn)) { - return Unwrap::Continue; + return VNVisit::Continue; } VNFuncApp funcApp; @@ -6595,7 +6573,7 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) case VNF_HWI_LZCNT_X64_LeadingZeroCount: case VNF_HWI_BMI1_TrailingZeroCount: case VNF_HWI_BMI1_X64_TrailingZeroCount: - return Unwrap::Continue; + return VNVisit::Continue; #elif defined(TARGET_ARM64) case VNF_HWI_AdvSimd_PopCount: case VNF_HWI_AdvSimd_LeadingZeroCount: @@ -6603,7 +6581,7 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) case VNF_HWI_ArmBase_LeadingZeroCount: case VNF_HWI_ArmBase_Arm64_LeadingZeroCount: case VNF_HWI_ArmBase_Arm64_LeadingSignCount: - return Unwrap::Continue; + return VNVisit::Continue; #endif #endif // FEATURE_HW_INTRINSICS @@ -6611,9 +6589,9 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) break; } } - return Unwrap::Abort; + return VNVisit::Abort; }; - return VNUnwrapPhis(vn, unwrapper, /* maxDepth */ 2) == UnwrapResult::Completed; + return VNVisitReachingVNs(vn, unwrapper) == VNVisitResult::Completed; } GenTreeFlags ValueNumStore::GetHandleFlags(ValueNum vn) @@ -15190,103 +15168,3 @@ void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) } } } - -//-------------------------------------------------------------------------------- -// VNUnwrapPhis: given a VN, call the specified callback function on it. In case if the VN -// is a phi node, recursively call the callback function on all the VN arguments of that phi node. -// -// Arguments: -// vn - The VN to unwrap -// argVisitor - The callback function to call on the vn and its PHI arguments if any -// maxDepth - Maximum depth to handle recursive PHI chains. -1 means no limits. -// vnk - The kind of VN to use (Conservative or Liberal) -// -// Return Value: -// UnwrapResult::MaxDepthReached - we hit the maxDepth limit (for TP reasons) -// UnwrapResult::Aborted - any argVisitor returned Unwrap::Abort -// UnwrapResult::Completed - unwrap was successful (all argVisitor returned Unwrap::Continue) -// -template -ValueNumStore::UnwrapResult ValueNumStore::VNUnwrapPhis(ValueNum vn, - TArgVisitor argVisitor, - int maxDepth, - ValueNumKind vnk) -{ - SmallValueNumSet hashSet; - UnwrapResult result = VNUnwrapPhisInternal(vn, argVisitor, maxDepth, vnk, hashSet); - return result; -} - -//-------------------------------------------------------------------------------- -// VNUnwrapPhisInternal: internal worker for VNUnwrapPhis, see comments above. -// -template -ValueNumStore::UnwrapResult ValueNumStore::VNUnwrapPhisInternal( - ValueNum vn, TArgVisitor argVisitor, int maxDepth, ValueNumKind vnk, SmallValueNumSet& hashSet) -{ - assert(maxDepth >= -1); - - if (!hashSet.Add(m_pComp, vn)) - { - // Recursive PhiDef chain detected - return UnwrapResult::Completed; - } - - unsigned* ssaArgs; - unsigned numSsaArgs; - unsigned lclNum; - - VNPhiDef phiDef; - VNMemoryPhiDef memoryPhiDef; - if (GetPhiDef(vn, &phiDef)) - { - lclNum = phiDef.LclNum; - ssaArgs = phiDef.SsaArgs; - numSsaArgs = phiDef.NumArgs; - } - else if (GetMemoryPhiDef(vn, &memoryPhiDef)) - { - lclNum = BAD_VAR_NUM; - ssaArgs = memoryPhiDef.SsaArgs; - numSsaArgs = memoryPhiDef.NumArgs; - } - else - { - // Not a phi node - handle normally and return - return argVisitor(vn) == Unwrap::Abort ? UnwrapResult::Aborted : UnwrapResult::Completed; - } - - // At this point vn is a PHI definition node - - if (maxDepth == 0) - { - return UnwrapResult::MaxDepthReached; - } - - assert(ssaArgs != nullptr); - assert(numSsaArgs > 0); - - for (unsigned i = 0; i < numSsaArgs; i++) - { - ValueNum phiArgVN; - if (lclNum != BAD_VAR_NUM) - { - phiArgVN = m_pComp->lvaGetDesc(lclNum)->GetPerSsaData(ssaArgs[i])->m_vnPair.Get(vnk); - } - else - { - phiArgVN = m_pComp->GetMemoryPerSsaData(ssaArgs[i])->m_vnPair.Get(vnk); - } - - // maxDepth being -1 means we don't have depth limits - int newDepth = maxDepth == -1 ? -1 : maxDepth - 1; - - // TODO-VN: Consider getting rid of this recursion and using an ArrayStack instead. - UnwrapResult result = VNUnwrapPhisInternal(phiArgVN, argVisitor, newDepth, vnk, hashSet); - if (result != UnwrapResult::Completed) - { - return result; - } - } - return UnwrapResult::Completed; -} diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 9e4f472891c2b0..7f5a17e339dc44 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -529,28 +529,121 @@ class ValueNumStore void PeelOffsets(ValueNum* vn, target_ssize_t* offset); - enum class Unwrap + typedef JitHashTable, bool> ValueNumSet; + + class SmallValueNumSet + { + union + { + ValueNum m_inlineElements[4]; + ValueNumSet* m_set; + }; + unsigned m_numElements = 0; + + public: + unsigned Count() + { + return m_numElements; + } + + template + void ForEach(Func func) + { + if (m_numElements <= ArrLen(m_inlineElements)) + { + for (unsigned i = 0; i < m_numElements; i++) + { + func(m_inlineElements[i]); + } + } + else + { + for (ValueNum vn : ValueNumSet::KeyIteration(m_set)) + { + func(vn); + } + } + } + + // Returns false if the value wasn't found + bool Lookup(ValueNum vn); + + // Returns false if the value already exists + bool Add(Compiler* comp, ValueNum vn); + }; + + enum class VNVisit { Continue, Abort, }; - enum class UnwrapResult + + enum class VNVisitResult { Completed, Aborted, MaxDepthReached, }; - template - UnwrapResult VNUnwrapPhis(ValueNum vn, - TArgVisitor argVisitor, - int maxDepth = -1, - ValueNumKind vnk = VNK_Conservative); -private: + ValueNum VNPhiDefToVN(const VNPhiDef& phiDef, unsigned ssaArgNum); + + //-------------------------------------------------------------------------------- + // VNVisitReachingVNs: given a VN, call the specified callback function on it and all the VNs that reach it + // via PHI definitions if any. + // + // Arguments: + // vn - The VN to visit all the reaching VNs for + // argVisitor - The callback function to call on the vn and its PHI arguments if any + // + // Return Value: + // VNVisitResult::Aborted - an argVisitor returned VNVisit::Abort, we stop the walk and return + // VNVisitResult::Completed - all argVisitor returned VNVisit::Continue + // template - UnwrapResult VNUnwrapPhisInternal( - ValueNum vn, TArgVisitor argVisitor, int maxDepth, ValueNumKind vnk, class SmallValueNumSet& hashSet); -public: + VNVisitResult VNVisitReachingVNs(ValueNum vn, TArgVisitor argVisitor) + { + JITDUMP("Starting VNVisitReachingVNs for " FMT_VN "\n", vn); + + ArrayStack toVisit(m_alloc); + toVisit.Push(vn); + + SmallValueNumSet visited; + visited.Add(m_pComp, vn); + while (toVisit.Height() > 0) + { + ValueNum vnToVisit = toVisit.Pop(); + + // We need to handle nested (and, potentially, recursive) phi definitions. + // For now, we ignore memory phi definitions. + VNPhiDef phiDef; + if (GetPhiDef(vnToVisit, &phiDef)) + { + for (unsigned ssaArgNum = 0; ssaArgNum < phiDef.NumArgs; ssaArgNum++) + { + ValueNum childVN = VNPhiDefToVN(phiDef, ssaArgNum); + if (visited.Add(m_pComp, childVN)) + { + toVisit.Push(childVN); + } + else + { + JITDUMP("Skipping already visited VN (cycle?) " FMT_VN "\n", childVN); + } + } + } + else + { + if (argVisitor(vnToVisit) == VNVisit::Abort) + { + // The visitor wants to abort the walk. + return VNVisitResult::Aborted; + } + } + } + + JITDUMP("Completed VNVisitReachingVNs successfully for " FMT_VN "\n", vn); + return VNVisitResult::Completed; + } // And the single constant for an object reference type. static ValueNum VNForNull() From 3f2443520da9df1ce15eb7865bac8941fdfe1c73 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 24 Jul 2024 21:10:58 +0200 Subject: [PATCH 09/10] fix build --- src/coreclr/jit/valuenum.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 7f5a17e339dc44..1587d42d8fce68 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -602,8 +602,6 @@ class ValueNumStore template VNVisitResult VNVisitReachingVNs(ValueNum vn, TArgVisitor argVisitor) { - JITDUMP("Starting VNVisitReachingVNs for " FMT_VN "\n", vn); - ArrayStack toVisit(m_alloc); toVisit.Push(vn); @@ -625,10 +623,6 @@ class ValueNumStore { toVisit.Push(childVN); } - else - { - JITDUMP("Skipping already visited VN (cycle?) " FMT_VN "\n", childVN); - } } } else @@ -640,8 +634,6 @@ class ValueNumStore } } } - - JITDUMP("Completed VNVisitReachingVNs successfully for " FMT_VN "\n", vn); return VNVisitResult::Completed; } From 9d3cff2913245a5e18efd3caf28095ce2f8c31f1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 24 Jul 2024 22:42:27 +0200 Subject: [PATCH 10/10] address feedback --- src/coreclr/jit/valuenum.cpp | 7 +++++-- src/coreclr/jit/valuenum.h | 17 +++++------------ 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c162d108c9b4e5..2e88aef9bb93d5 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6526,7 +6526,7 @@ bool ValueNumStore::IsVNInt32Constant(ValueNum vn) bool ValueNumStore::IsVNNeverNegative(ValueNum vn) { - auto unwrapper = [this](ValueNum vn) -> VNVisit { + auto vnVisitor = [this](ValueNum vn) -> VNVisit { if ((vn == NoVN) || !varTypeIsIntegral(TypeOfVN(vn))) { return VNVisit::Abort; @@ -6552,6 +6552,9 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) return VNVisit::Continue; } + // TODO-VN: Recognize Span.Length + // Handle more intrinsics such as Math.Max(neverNegative1, neverNegative2) + VNFuncApp funcApp; if (GetVNFunc(vn, &funcApp)) { @@ -6591,7 +6594,7 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) } return VNVisit::Abort; }; - return VNVisitReachingVNs(vn, unwrapper) == VNVisitResult::Completed; + return VNVisitReachingVNs(vn, vnVisitor) == VNVisit::Continue; } GenTreeFlags ValueNumStore::GetHandleFlags(ValueNum vn) diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 1587d42d8fce68..46d7378f185c61 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -578,13 +578,6 @@ class ValueNumStore Abort, }; - enum class VNVisitResult - { - Completed, - Aborted, - MaxDepthReached, - }; - ValueNum VNPhiDefToVN(const VNPhiDef& phiDef, unsigned ssaArgNum); //-------------------------------------------------------------------------------- @@ -596,11 +589,11 @@ class ValueNumStore // argVisitor - The callback function to call on the vn and its PHI arguments if any // // Return Value: - // VNVisitResult::Aborted - an argVisitor returned VNVisit::Abort, we stop the walk and return - // VNVisitResult::Completed - all argVisitor returned VNVisit::Continue + // VNVisit::Aborted - an argVisitor returned VNVisit::Abort, we stop the walk and return + // VNVisit::Continue - all argVisitor returned VNVisit::Continue // template - VNVisitResult VNVisitReachingVNs(ValueNum vn, TArgVisitor argVisitor) + VNVisit VNVisitReachingVNs(ValueNum vn, TArgVisitor argVisitor) { ArrayStack toVisit(m_alloc); toVisit.Push(vn); @@ -630,11 +623,11 @@ class ValueNumStore if (argVisitor(vnToVisit) == VNVisit::Abort) { // The visitor wants to abort the walk. - return VNVisitResult::Aborted; + return VNVisit::Abort; } } } - return VNVisitResult::Completed; + return VNVisit::Continue; } // And the single constant for an object reference type.