From 2e72afe6f01612b50612cde45e624d07d53cf4a1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jul 2023 15:46:51 +0200 Subject: [PATCH 1/6] JIT: Cache memory dependencies for VN map selects Hoisting relies on being able to look at which memory dependencies a candidate is dependent upon. VN tracks these during the map select logic; however, it fails to do so when the map selection hits the cache. This changes the logic to make sure the memory dependencies are cached. Fix #75442 --- src/coreclr/jit/optimizer.cpp | 5 +- src/coreclr/jit/valuenum.cpp | 577 ++++++++++-------- src/coreclr/jit/valuenum.h | 39 +- src/coreclr/jit/valuenumfuncs.h | 3 + .../JitBlue/Runtime_75442/Runtime_75442.cs | 62 ++ .../Runtime_75442/Runtime_75442.csproj | 8 + 6 files changed, 442 insertions(+), 252 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.csproj diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 1b62867cb14310..684f71b898f313 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -7109,10 +7109,7 @@ void Compiler::optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, V // unsigned const loopNum = block->bbNatLoopNum; - if (loopNum == BasicBlock::NOT_IN_LOOP) - { - return; - } + assert(loopNum != BasicBlock::NOT_IN_LOOP); // Find the loop associated with this memory VN. // diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0cb86cbd04e756..1f87880934aef8 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2819,12 +2819,7 @@ ValueNum ValueNumStore::VNForMapSelect(ValueNumKind vnk, var_types type, ValueNu { assert(MapIsPrecise(map)); - int budget = m_mapSelectBudget; - bool usedRecursiveVN = false; - ValueNum result = VNForMapSelectWork(vnk, type, map, index, &budget, &usedRecursiveVN); - - // The remaining budget should always be between [0..m_mapSelectBudget] - assert((budget >= 0) && (budget <= m_mapSelectBudget)); + ValueNum result = VNForMapSelectInner(vnk, type, map, index); JITDUMP(" VNForMapSelect(" FMT_VN ", " FMT_VN "):%s returns ", map, index, VNMapTypeName(type)); JITDUMPEXEC(m_pComp->vnPrint(result, 1)); @@ -2853,13 +2848,8 @@ ValueNum ValueNumStore::VNForMapPhysicalSelect( { assert(MapIsPhysical(map)); - ValueNum selector = EncodePhysicalSelector(offset, size); - int budget = m_mapSelectBudget; - bool usedRecursiveVN = false; - ValueNum result = VNForMapSelectWork(vnk, type, map, selector, &budget, &usedRecursiveVN); - - // The remaining budget should always be between [0..m_mapSelectBudget] - assert((budget >= 0) && (budget <= m_mapSelectBudget)); + ValueNum selector = EncodePhysicalSelector(offset, size); + ValueNum result = VNForMapSelectInner(vnk, type, map, selector); JITDUMP(" VNForMapPhysicalSelect(" FMT_VN ", ", map); JITDUMPEXEC(vnDumpPhysicalSelector(selector)); @@ -2870,17 +2860,101 @@ ValueNum ValueNumStore::VNForMapPhysicalSelect( return result; } +//------------------------------------------------------------------------------ +// VNForMapSelectInner: Select value from a map and record loop memory dependencies. +// +// Arguments: +// vnk - Value number kind (see the notes for "VNForMapSelect") +// type - The type to select +// map - (VN of) the physical map +// index - The selector +// +// Return Value: +// Value number for the result of the evaluation. +// +ValueNum ValueNumStore::VNForMapSelectInner(ValueNumKind vnk, var_types type, ValueNum map, ValueNum index) +{ + int budget = m_mapSelectBudget; + bool usedRecursiveVN = false; + ArrayStack memoryDependencies(m_alloc); + ValueNum result = VNForMapSelectWork(vnk, type, map, index, &budget, &usedRecursiveVN, &memoryDependencies); + + // The remaining budget should always be between [0..m_mapSelectBudget] + assert((budget >= 0) && (budget <= m_mapSelectBudget)); + + // If the current tree is in a loop then record memory dependencies for hoisting. + if (m_pComp->compCurBB->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) + { + for (int i = 0; i < memoryDependencies.Height(); i++) + { + m_pComp->optRecordLoopMemoryDependence(m_pComp->compCurTree, m_pComp->compCurBB, + memoryDependencies.Bottom(i)); + } + } + + return result; +} + +//------------------------------------------------------------------------------ +// SetMemoryDependencies: Set the cached memory dependencies for a map-select +// cache entry. +// +// Arguments: +// alloc - Allocator to use if memory is required. +// deps - Array stack containing the memory dependencies. +// startIndex - Start index into 'deps' of memory dependencies. +// +void ValueNumStore::MapSelectWorkCacheEntry::SetMemoryDependencies(CompAllocator alloc, + ArrayStack& deps, + unsigned startIndex) +{ + m_numMemoryDependencies = deps.Height() - startIndex; + ValueNum* arr; + if (m_numMemoryDependencies > ArrLen(m_inlineMemoryDependencies)) + { + m_memoryDependencies = new (alloc) ValueNum[m_numMemoryDependencies]; + + arr = m_memoryDependencies; + } + else + { + arr = m_inlineMemoryDependencies; + } + + for (unsigned i = 0; i < m_numMemoryDependencies; i++) + arr[i] = deps.Bottom(startIndex + i); +} + +//------------------------------------------------------------------------------ +// GetMemoryDependencies: Push all of the memory dependencies cached in this +// entry into the specified array stack. +// +// Arguments: +// result - Array stack to push memory dependencies into. +// +void ValueNumStore::MapSelectWorkCacheEntry::GetMemoryDependencies(ArrayStack& result) +{ + ValueNum* arr = m_numMemoryDependencies <= ArrLen(m_inlineMemoryDependencies) ? m_inlineMemoryDependencies + : m_memoryDependencies; + + for (unsigned i = 0; i < m_numMemoryDependencies; i++) + { + result.Push(arr[i]); + } +} + //------------------------------------------------------------------------------ // VNForMapSelectWork : A method that does the work for VNForMapSelect and may call itself recursively. // // Arguments: -// vnk - Value number kind -// type - Value type -// map - The map to select from -// index - The selector -// pBudget - Remaining budget for the outer evaluation -// pUsedRecursiveVN - Out-parameter that is set to true iff RecursiveVN was returned from this method -// or from a method called during one of recursive invocations. +// vnk - Value number kind +// type - Value type +// map - The map to select from +// index - The selector +// pBudget - Remaining budget for the outer evaluation +// pUsedRecursiveVN - Out-parameter that is set to true iff RecursiveVN was returned from this method +// or from a method called during one of recursive invocations. +// memoryDependencies - Array stack that records VNs of memories that the result is dependent upon. // // Return Value: // Value number for the result of the evaluation. @@ -2890,8 +2964,13 @@ ValueNum ValueNumStore::VNForMapPhysicalSelect( // "select(m1, ind)", ..., "select(mk, ind)" to see if they agree. It needs to know which kind of value number // (liberal/conservative) to read from the SSA def referenced in the phi argument. // -ValueNum ValueNumStore::VNForMapSelectWork( - ValueNumKind vnk, var_types type, ValueNum map, ValueNum index, int* pBudget, bool* pUsedRecursiveVN) +ValueNum ValueNumStore::VNForMapSelectWork(ValueNumKind vnk, + var_types type, + ValueNum map, + ValueNum index, + int* pBudget, + bool* pUsedRecursiveVN, + ArrayStack* memoryDependencies) { TailCall: // This label allows us to directly implement a tail call by setting up the arguments, and doing a goto to here. @@ -2912,304 +2991,310 @@ ValueNum ValueNumStore::VNForMapSelectWork( unsigned selLim = JitConfig.JitVNMapSelLimit(); assert(selLim == 0 || m_numMapSels < selLim); #endif - ValueNum res; + + int firstMemoryDependency = memoryDependencies->Height(); + MapSelectWorkCacheEntry entry; VNDefFuncApp<2> fstruct(VNF_MapSelect, map, index); - if (GetVNFunc2Map()->Lookup(fstruct, &res)) + if (GetMapSelectWorkCache()->Lookup(fstruct, &entry)) { - return res; + entry.GetMemoryDependencies(*memoryDependencies); + return entry.Result; } - else + + // Give up if we've run out of budget. + if (*pBudget == 0) { - // Give up if we've run out of budget. - if (*pBudget == 0) - { - // We have to use 'nullptr' for the basic block here, because subsequent expressions - // in different blocks may find this result in the VNFunc2Map -- other expressions in - // the IR may "evaluate" to this same VNForExpr, so it is not "unique" in the sense - // that permits the BasicBlock attribution. - res = VNForExpr(nullptr, type); - GetVNFunc2Map()->Set(fstruct, res); - return res; - } + // We have to use 'nullptr' for the basic block here, because subsequent expressions + // in different blocks may find this result in the VNFunc2Map -- other expressions in + // the IR may "evaluate" to this same VNForExpr, so it is not "unique" in the sense + // that permits the BasicBlock attribution. + entry.Result = VNForExpr(nullptr, type); + GetMapSelectWorkCache()->Set(fstruct, entry); + return entry.Result; + } - // Reduce our budget by one - (*pBudget)--; + // Reduce our budget by one + (*pBudget)--; - // If it's recursive, stop the recursion. - if (SelectIsBeingEvaluatedRecursively(map, index)) - { - *pUsedRecursiveVN = true; - return RecursiveVN; - } + // If it's recursive, stop the recursion. + if (SelectIsBeingEvaluatedRecursively(map, index)) + { + *pUsedRecursiveVN = true; + return RecursiveVN; + } - VNFuncApp funcApp; - if (GetVNFunc(map, &funcApp)) + VNFuncApp funcApp; + if (GetVNFunc(map, &funcApp)) + { + switch (funcApp.m_func) { - switch (funcApp.m_func) + case VNF_MapStore: { - case VNF_MapStore: - { - assert(MapIsPrecise(map)); + assert(MapIsPrecise(map)); - // select(store(m, i, v), i) == v - if (funcApp.m_args[1] == index) - { + // select(store(m, i, v), i) == v + if (funcApp.m_args[1] == index) + { #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" AX1: select([" FMT_VN "]store(" FMT_VN ", " FMT_VN ", " FMT_VN "), " FMT_VN - ") ==> " FMT_VN ".\n", - funcApp.m_args[0], map, funcApp.m_args[1], funcApp.m_args[2], index, funcApp.m_args[2]); + JITDUMP(" AX1: select([" FMT_VN "]store(" FMT_VN ", " FMT_VN ", " FMT_VN "), " FMT_VN + ") ==> " FMT_VN ".\n", + funcApp.m_args[0], map, funcApp.m_args[1], funcApp.m_args[2], index, funcApp.m_args[2]); #endif - m_pComp->optRecordLoopMemoryDependence(m_pComp->compCurTree, m_pComp->compCurBB, - funcApp.m_args[0]); - return funcApp.m_args[2]; - } - // i # j ==> select(store(m, i, v), j) == select(m, j) - // Currently the only source of distinctions is when both indices are constants. - else if (IsVNConstant(index) && IsVNConstant(funcApp.m_args[1])) - { - assert(funcApp.m_args[1] != index); // we already checked this above. + memoryDependencies->Push(funcApp.m_args[0]); + + return funcApp.m_args[2]; + } + // i # j ==> select(store(m, i, v), j) == select(m, j) + // Currently the only source of distinctions is when both indices are constants. + else if (IsVNConstant(index) && IsVNConstant(funcApp.m_args[1])) + { + assert(funcApp.m_args[1] != index); // we already checked this above. #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" AX2: " FMT_VN " != " FMT_VN " ==> select([" FMT_VN "]store(" FMT_VN ", " FMT_VN - ", " FMT_VN "), " FMT_VN ") ==> select(" FMT_VN ", " FMT_VN - ") remaining budget is %d.\n", - index, funcApp.m_args[1], map, funcApp.m_args[0], funcApp.m_args[1], funcApp.m_args[2], - index, funcApp.m_args[0], index, *pBudget); + JITDUMP(" AX2: " FMT_VN " != " FMT_VN " ==> select([" FMT_VN "]store(" FMT_VN ", " FMT_VN + ", " FMT_VN "), " FMT_VN ") ==> select(" FMT_VN ", " FMT_VN ") remaining budget is %d.\n", + index, funcApp.m_args[1], map, funcApp.m_args[0], funcApp.m_args[1], funcApp.m_args[2], + index, funcApp.m_args[0], index, *pBudget); #endif - // This is the equivalent of the recursive tail call: - // return VNForMapSelect(vnk, typ, funcApp.m_args[0], index); - // Make sure we capture any exceptions from the "i" and "v" of the store... - map = funcApp.m_args[0]; - goto TailCall; - } + // This is the equivalent of the recursive tail call: + // return VNForMapSelect(vnk, typ, funcApp.m_args[0], index); + // Make sure we capture any exceptions from the "i" and "v" of the store... + map = funcApp.m_args[0]; + goto TailCall; } - break; + } + break; - case VNF_MapPhysicalStore: - { - assert(MapIsPhysical(map)); + case VNF_MapPhysicalStore: + { + assert(MapIsPhysical(map)); #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" select("); - JITDUMPEXEC(m_pComp->vnPrint(map, 1)); - JITDUMP(", "); - JITDUMPEXEC(vnDumpPhysicalSelector(index)); - JITDUMP(")"); + JITDUMP(" select("); + JITDUMPEXEC(m_pComp->vnPrint(map, 1)); + JITDUMP(", "); + JITDUMPEXEC(vnDumpPhysicalSelector(index)); + JITDUMP(")"); #endif - ValueNum storeSelector = funcApp.m_args[1]; + ValueNum storeSelector = funcApp.m_args[1]; - if (index == storeSelector) - { + if (index == storeSelector) + { #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" ==> " FMT_VN "\n", funcApp.m_args[2]); + JITDUMP(" ==> " FMT_VN "\n", funcApp.m_args[2]); #endif - return funcApp.m_args[2]; - } + return funcApp.m_args[2]; + } - unsigned selectSize; - unsigned selectOffset = DecodePhysicalSelector(index, &selectSize); + unsigned selectSize; + unsigned selectOffset = DecodePhysicalSelector(index, &selectSize); - unsigned storeSize; - unsigned storeOffset = DecodePhysicalSelector(storeSelector, &storeSize); + unsigned storeSize; + unsigned storeOffset = DecodePhysicalSelector(storeSelector, &storeSize); - unsigned selectEndOffset = selectOffset + selectSize; // Exclusive. - unsigned storeEndOffset = storeOffset + storeSize; // Exclusive. + unsigned selectEndOffset = selectOffset + selectSize; // Exclusive. + unsigned storeEndOffset = storeOffset + storeSize; // Exclusive. - if ((storeOffset <= selectOffset) && (selectEndOffset <= storeEndOffset)) - { + if ((storeOffset <= selectOffset) && (selectEndOffset <= storeEndOffset)) + { #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" ==> enclosing, selecting inner, remaining budget is %d\n", *pBudget); + JITDUMP(" ==> enclosing, selecting inner, remaining budget is %d\n", *pBudget); #endif - map = funcApp.m_args[2]; - index = EncodePhysicalSelector(selectOffset - storeOffset, selectSize); - goto TailCall; - } + map = funcApp.m_args[2]; + index = EncodePhysicalSelector(selectOffset - storeOffset, selectSize); + goto TailCall; + } - // If it was disjoint with the location being selected, continue the linear search. - if ((storeEndOffset <= selectOffset) || (selectEndOffset <= storeOffset)) - { + // If it was disjoint with the location being selected, continue the linear search. + if ((storeEndOffset <= selectOffset) || (selectEndOffset <= storeOffset)) + { #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" ==> disjoint, remaining budget is %d\n", *pBudget); + JITDUMP(" ==> disjoint, remaining budget is %d\n", *pBudget); #endif - map = funcApp.m_args[0]; - goto TailCall; - } - else - { + map = funcApp.m_args[0]; + goto TailCall; + } + else + { #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" ==> aliasing!\n"); + JITDUMP(" ==> aliasing!\n"); #endif - } } - break; + } + break; - case VNF_BitCast: - assert(MapIsPhysical(map)); + case VNF_BitCast: + assert(MapIsPhysical(map)); #if FEATURE_VN_TRACE_APPLY_SELECTORS - JITDUMP(" select(bitcast<%s>(" FMT_VN ")) ==> select(" FMT_VN ")\n", - varTypeName(TypeOfVN(funcApp.m_args[0])), funcApp.m_args[0], funcApp.m_args[0]); + JITDUMP(" select(bitcast<%s>(" FMT_VN ")) ==> select(" FMT_VN ")\n", + varTypeName(TypeOfVN(funcApp.m_args[0])), funcApp.m_args[0], funcApp.m_args[0]); #endif // FEATURE_VN_TRACE_APPLY_SELECTORS - map = funcApp.m_args[0]; - goto TailCall; + map = funcApp.m_args[0]; + goto TailCall; - case VNF_ZeroObj: - assert(MapIsPhysical(map)); + case VNF_ZeroObj: + assert(MapIsPhysical(map)); - // TODO-CQ: support selection of TYP_STRUCT here. - if (type != TYP_STRUCT) - { - return VNZeroForType(type); - } - break; + // TODO-CQ: support selection of TYP_STRUCT here. + if (type != TYP_STRUCT) + { + return VNZeroForType(type); + } + break; - case VNF_PhiDef: - case VNF_PhiMemoryDef: + case VNF_PhiDef: + case VNF_PhiMemoryDef: + { + unsigned lclNum = BAD_VAR_NUM; + bool isMemory = false; + VNFuncApp phiFuncApp; + bool defArgIsFunc = false; + if (funcApp.m_func == VNF_PhiDef) + { + lclNum = unsigned(funcApp.m_args[0]); + defArgIsFunc = GetVNFunc(funcApp.m_args[2], &phiFuncApp); + } + else + { + assert(funcApp.m_func == VNF_PhiMemoryDef); + isMemory = true; + defArgIsFunc = GetVNFunc(funcApp.m_args[1], &phiFuncApp); + } + if (defArgIsFunc && phiFuncApp.m_func == VNF_Phi) { - unsigned lclNum = BAD_VAR_NUM; - bool isMemory = false; - VNFuncApp phiFuncApp; - bool defArgIsFunc = false; - if (funcApp.m_func == VNF_PhiDef) + // select(phi(m1, m2), x): if select(m1, x) == select(m2, x), return that, else new fresh. + // Get the first argument of the phi. + + // We need to be careful about breaking infinite recursion. Record the outer select. + m_fixedPointMapSels.Push(VNDefFuncApp<2>(VNF_MapSelect, map, index)); + + assert(IsVNConstant(phiFuncApp.m_args[0])); + unsigned phiArgSsaNum = ConstantValue(phiFuncApp.m_args[0]); + ValueNum phiArgVN; + if (isMemory) { - lclNum = unsigned(funcApp.m_args[0]); - defArgIsFunc = GetVNFunc(funcApp.m_args[2], &phiFuncApp); + phiArgVN = m_pComp->GetMemoryPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); } else { - assert(funcApp.m_func == VNF_PhiMemoryDef); - isMemory = true; - defArgIsFunc = GetVNFunc(funcApp.m_args[1], &phiFuncApp); + phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); } - if (defArgIsFunc && phiFuncApp.m_func == VNF_Phi) + if (phiArgVN != ValueNumStore::NoVN) { - // select(phi(m1, m2), x): if select(m1, x) == select(m2, x), return that, else new fresh. - // Get the first argument of the phi. - - // We need to be careful about breaking infinite recursion. Record the outer select. - m_fixedPointMapSels.Push(VNDefFuncApp<2>(VNF_MapSelect, map, index)); - - assert(IsVNConstant(phiFuncApp.m_args[0])); - unsigned phiArgSsaNum = ConstantValue(phiFuncApp.m_args[0]); - ValueNum phiArgVN; - if (isMemory) + bool allSame = true; + ValueNum argRest = phiFuncApp.m_args[1]; + ValueNum sameSelResult = VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, + pUsedRecursiveVN, memoryDependencies); + + // It is possible that we just now exceeded our budget, if so we need to force an early exit + // and stop calling VNForMapSelectWork + if (*pBudget <= 0) { - phiArgVN = m_pComp->GetMemoryPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); + // We don't have any budget remaining to verify that all phiArgs are the same + // so setup the default failure case now. + allSame = false; } - else - { - phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); - } - if (phiArgVN != ValueNumStore::NoVN) + + while (allSame && argRest != ValueNumStore::NoVN) { - bool allSame = true; - ValueNum argRest = phiFuncApp.m_args[1]; - ValueNum sameSelResult = - VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, pUsedRecursiveVN); - - // It is possible that we just now exceeded our budget, if so we need to force an early exit - // and stop calling VNForMapSelectWork - if (*pBudget <= 0) + ValueNum cur = argRest; + VNFuncApp phiArgFuncApp; + if (GetVNFunc(argRest, &phiArgFuncApp) && phiArgFuncApp.m_func == VNF_Phi) + { + cur = phiArgFuncApp.m_args[0]; + argRest = phiArgFuncApp.m_args[1]; + } + else + { + argRest = ValueNumStore::NoVN; // Cause the loop to terminate. + } + assert(IsVNConstant(cur)); + phiArgSsaNum = ConstantValue(cur); + if (isMemory) + { + phiArgVN = m_pComp->GetMemoryPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); + } + else + { + phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); + } + if (phiArgVN == ValueNumStore::NoVN) { - // We don't have any budget remaining to verify that all phiArgs are the same - // so setup the default failure case now. allSame = false; } - - while (allSame && argRest != ValueNumStore::NoVN) + else { - ValueNum cur = argRest; - VNFuncApp phiArgFuncApp; - if (GetVNFunc(argRest, &phiArgFuncApp) && phiArgFuncApp.m_func == VNF_Phi) - { - cur = phiArgFuncApp.m_args[0]; - argRest = phiArgFuncApp.m_args[1]; - } - else - { - argRest = ValueNumStore::NoVN; // Cause the loop to terminate. - } - assert(IsVNConstant(cur)); - phiArgSsaNum = ConstantValue(cur); - if (isMemory) - { - phiArgVN = m_pComp->GetMemoryPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); - } - else + bool usedRecursiveVN = false; + ValueNum curResult = VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, + &usedRecursiveVN, memoryDependencies); + + *pUsedRecursiveVN |= usedRecursiveVN; + if (sameSelResult == ValueNumStore::RecursiveVN) { - phiArgVN = m_pComp->lvaTable[lclNum].GetPerSsaData(phiArgSsaNum)->m_vnPair.Get(vnk); + sameSelResult = curResult; } - if (phiArgVN == ValueNumStore::NoVN) + if (curResult != ValueNumStore::RecursiveVN && curResult != sameSelResult) { allSame = false; } - else - { - bool usedRecursiveVN = false; - ValueNum curResult = - VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, &usedRecursiveVN); - - *pUsedRecursiveVN |= usedRecursiveVN; - if (sameSelResult == ValueNumStore::RecursiveVN) - { - sameSelResult = curResult; - } - if (curResult != ValueNumStore::RecursiveVN && curResult != sameSelResult) - { - allSame = false; - } - } } - if (allSame && sameSelResult != ValueNumStore::RecursiveVN) + } + if (allSame && sameSelResult != ValueNumStore::RecursiveVN) + { + // Make sure we're popping what we pushed. + assert(FixedPointMapSelsTopHasValue(map, index)); + m_fixedPointMapSels.Pop(); + + // To avoid exponential searches, we make sure that this result is memo-ized. + // The result is always valid for memoization if we didn't rely on RecursiveVN to get + // it. + // If RecursiveVN was used, we are processing a loop and we can't memo-ize this + // intermediate + // result if, e.g., this block is in a multi-entry loop. + if (!*pUsedRecursiveVN) { - // Make sure we're popping what we pushed. - assert(FixedPointMapSelsTopHasValue(map, index)); - m_fixedPointMapSels.Pop(); - - // To avoid exponential searches, we make sure that this result is memo-ized. - // The result is always valid for memoization if we didn't rely on RecursiveVN to get - // it. - // If RecursiveVN was used, we are processing a loop and we can't memo-ize this - // intermediate - // result if, e.g., this block is in a multi-entry loop. - if (!*pUsedRecursiveVN) - { - GetVNFunc2Map()->Set(fstruct, sameSelResult); - } + entry.Result = sameSelResult; + entry.SetMemoryDependencies(m_alloc, *memoryDependencies, firstMemoryDependency); - return sameSelResult; + GetMapSelectWorkCache()->Set(fstruct, entry); } - // Otherwise, fall through to creating the select(phi(m1, m2), x) function application. + + return sameSelResult; } - // Make sure we're popping what we pushed. - assert(FixedPointMapSelsTopHasValue(map, index)); - m_fixedPointMapSels.Pop(); + // Otherwise, fall through to creating the select(phi(m1, m2), x) function application. } + // Make sure we're popping what we pushed. + assert(FixedPointMapSelsTopHasValue(map, index)); + m_fixedPointMapSels.Pop(); } - break; - - default: - break; } + break; + + default: + break; } + } - // We may have run out of budget and already assigned a result - if (!GetVNFunc2Map()->Lookup(fstruct, &res)) - { - // Otherwise, assign a new VN for the function application. - Chunk* const c = GetAllocChunk(type, CEA_Func2); - unsigned const offsetWithinChunk = c->AllocVN(); - VNDefFuncAppFlexible* fapp = c->PointerToFuncApp(offsetWithinChunk, 2); - fapp->m_func = fstruct.m_func; - fapp->m_args[0] = fstruct.m_args[0]; - fapp->m_args[1] = fstruct.m_args[1]; - res = c->m_baseVN + offsetWithinChunk; + // We may have run out of budget and already assigned a result + if (!GetMapSelectWorkCache()->Lookup(fstruct, &entry)) + { + // Otherwise, assign a new VN for the function application. + Chunk* const c = GetAllocChunk(type, CEA_Func2); + unsigned const offsetWithinChunk = c->AllocVN(); + VNDefFuncAppFlexible* fapp = c->PointerToFuncApp(offsetWithinChunk, 2); + fapp->m_func = fstruct.m_func; + fapp->m_args[0] = fstruct.m_args[0]; + fapp->m_args[1] = fstruct.m_args[1]; - GetVNFunc2Map()->Set(fstruct, res); - } - return res; + entry.Result = c->m_baseVN + offsetWithinChunk; + entry.SetMemoryDependencies(m_alloc, *memoryDependencies, firstMemoryDependency); + + GetMapSelectWorkCache()->Set(fstruct, entry); } + + return entry.Result; } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index cb7be434314180..ee8ab34fb3fbf7 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -681,9 +681,16 @@ class ValueNumStore ValueNum VNForMapPhysicalSelect(ValueNumKind vnk, var_types type, ValueNum map, unsigned offset, unsigned size); + ValueNum VNForMapSelectInner(ValueNumKind vnk, var_types type, ValueNum map, ValueNum index); + // A method that does the work for VNForMapSelect and may call itself recursively. - ValueNum VNForMapSelectWork( - ValueNumKind vnk, var_types type, ValueNum map, ValueNum index, int* pBudget, bool* pUsedRecursiveVN); + ValueNum VNForMapSelectWork(ValueNumKind vnk, + var_types type, + ValueNum map, + ValueNum index, + int* pBudget, + bool* pUsedRecursiveVN, + ArrayStack* loopMemoryDependencies); // A specialized version of VNForFunc that is used for VNF_MapStore and provides some logging when verbose is set ValueNum VNForMapStore(ValueNum map, ValueNum index, ValueNum value); @@ -1802,12 +1809,40 @@ class ValueNumStore return m_VNFunc4Map; } + class MapSelectWorkCacheEntry + { + union { + ValueNum* m_memoryDependencies; + ValueNum m_inlineMemoryDependencies[sizeof(ValueNum*) / sizeof(ValueNum)]; + }; + + unsigned m_numMemoryDependencies = 0; + + public: + ValueNum Result; + + void SetMemoryDependencies(CompAllocator alloc, ArrayStack& deps, unsigned startIndex); + void GetMemoryDependencies(ArrayStack& deps); + }; + + typedef JitHashTable, VNDefFuncAppKeyFuncs<2>, MapSelectWorkCacheEntry> MapSelectWorkCache; + MapSelectWorkCache* m_mapSelectWorkCache = nullptr; + MapSelectWorkCache* GetMapSelectWorkCache() + { + if (m_mapSelectWorkCache == nullptr) + { + m_mapSelectWorkCache = new (m_alloc) MapSelectWorkCache(m_alloc); + } + return m_mapSelectWorkCache; + } + // We reserve Chunk 0 for "special" VNs. enum SpecialRefConsts { SRC_Null, SRC_Void, SRC_EmptyExcSet, + SRC_EmptyMemDepSet, SRC_NumSpecialRefConsts }; diff --git a/src/coreclr/jit/valuenumfuncs.h b/src/coreclr/jit/valuenumfuncs.h index f875f3aadaf43d..6f8d65c9719186 100644 --- a/src/coreclr/jit/valuenumfuncs.h +++ b/src/coreclr/jit/valuenumfuncs.h @@ -16,6 +16,9 @@ ValueNumFuncDef(PhiDef, 3, false, false, false, false) // Args: 0: l ValueNumFuncDef(PhiMemoryDef, 2, false, false, false, false) // Args: 0: VN for basic block pointer, 1: VN of definition ValueNumFuncDef(Phi, 2, false, false, false, false) // A phi function. Only occurs as arg of PhiDef or PhiMemoryDef. Arguments are SSA numbers of var being defined. +ValueNumFuncDef(ValWithMem, 2, false, false, false, false) // Args: 0: value number; 1: value number representing memory dependencies +ValueNumFuncDef(MemDepSetCons, 2, false, false, false, false) // Args: 0: memory VN; 1: Memory dependency set + ValueNumFuncDef(PtrToLoc, 2, false, true, false, false) // Pointer (byref) to a local variable. Args: VN's of: 0: local's number, 1: offset. ValueNumFuncDef(PtrToArrElem, 4, false, false, false, false) // Pointer (byref) to an array element. Args: 0: array elem type eq class var_types value, VN's of: 1: array, 2: index, 3: offset. ValueNumFuncDef(PtrToStatic, 3, false, true, false, false) // Pointer (byref) to a static variable (or possibly a field thereof, if the static variable is a struct). diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs b/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs new file mode 100644 index 00000000000000..d6c35c1e493c17 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs @@ -0,0 +1,62 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Generated by Fuzzlyn v1.5 on 2023-01-16 17:01:38 +// Run on X64 Windows +// Seed: 10178493004625664135 +// Reduced from 625.1 KiB to 1.5 KiB in 00:03:14 +// Debug: Outputs 0 +// Release: Outputs 1 +using Xunit; +using System; +using System.Runtime.CompilerServices; + +public struct S0 +{ + public bool F1; +} + +public struct S1 +{ + public S0 F2; +} + +public class Runtime_75442 +{ + public static ushort s_19; + public static S1 s_21; + public static ushort[] s_32; + + [Fact] + public static int TestEntryPoint() + { + ulong[] vr0 = new ulong[] { 0 }; + for (int vr1 = 0; vr1 < UpperBound(); vr1++) + { + vr0[0] = 1; + if (s_21.F2.F1) + { + s_32[0] = s_19; + } + + vr0[0] ^= vr0[0]; + Use(vr1); + } + + if (vr0[0] != 0) + { + Console.WriteLine("FAIL: vr0[0] == {0}", vr0[0]); + return 101; + } + + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int UpperBound() => 2; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Use(int val) + { + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.csproj new file mode 100644 index 00000000000000..15edd99711a1a4 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.csproj @@ -0,0 +1,8 @@ + + + True + + + + + \ No newline at end of file From 34490ca3364949beb021b745433d870054d32671 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jul 2023 16:10:11 +0200 Subject: [PATCH 2/6] Fix indentation in test --- .../JitBlue/Runtime_75442/Runtime_75442.cs | 72 +++++++++---------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs b/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs index d6c35c1e493c17..dc2177d8e870da 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75442/Runtime_75442.cs @@ -13,50 +13,50 @@ public struct S0 { - public bool F1; + public bool F1; } public struct S1 { - public S0 F2; + public S0 F2; } public class Runtime_75442 { - public static ushort s_19; - public static S1 s_21; - public static ushort[] s_32; + public static ushort s_19; + public static S1 s_21; + public static ushort[] s_32; [Fact] - public static int TestEntryPoint() - { - ulong[] vr0 = new ulong[] { 0 }; - for (int vr1 = 0; vr1 < UpperBound(); vr1++) - { - vr0[0] = 1; - if (s_21.F2.F1) - { - s_32[0] = s_19; - } - - vr0[0] ^= vr0[0]; - Use(vr1); - } - - if (vr0[0] != 0) - { - Console.WriteLine("FAIL: vr0[0] == {0}", vr0[0]); - return 101; - } - - return 100; - } - - [MethodImpl(MethodImplOptions.NoInlining)] - private static int UpperBound() => 2; - - [MethodImpl(MethodImplOptions.NoInlining)] - private static void Use(int val) - { - } + public static int TestEntryPoint() + { + ulong[] vr0 = new ulong[] { 0 }; + for (int vr1 = 0; vr1 < UpperBound(); vr1++) + { + vr0[0] = 1; + if (s_21.F2.F1) + { + s_32[0] = s_19; + } + + vr0[0] ^= vr0[0]; + Use(vr1); + } + + if (vr0[0] != 0) + { + Console.WriteLine("FAIL: vr0[0] == {0}", vr0[0]); + return 101; + } + + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int UpperBound() => 2; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Use(int val) + { + } } From f501e4d362c156468edf987f4107c26e124ae87c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jul 2023 16:10:53 +0200 Subject: [PATCH 3/6] Remove old code --- src/coreclr/jit/valuenumfuncs.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/jit/valuenumfuncs.h b/src/coreclr/jit/valuenumfuncs.h index 6f8d65c9719186..f875f3aadaf43d 100644 --- a/src/coreclr/jit/valuenumfuncs.h +++ b/src/coreclr/jit/valuenumfuncs.h @@ -16,9 +16,6 @@ ValueNumFuncDef(PhiDef, 3, false, false, false, false) // Args: 0: l ValueNumFuncDef(PhiMemoryDef, 2, false, false, false, false) // Args: 0: VN for basic block pointer, 1: VN of definition ValueNumFuncDef(Phi, 2, false, false, false, false) // A phi function. Only occurs as arg of PhiDef or PhiMemoryDef. Arguments are SSA numbers of var being defined. -ValueNumFuncDef(ValWithMem, 2, false, false, false, false) // Args: 0: value number; 1: value number representing memory dependencies -ValueNumFuncDef(MemDepSetCons, 2, false, false, false, false) // Args: 0: memory VN; 1: Memory dependency set - ValueNumFuncDef(PtrToLoc, 2, false, true, false, false) // Pointer (byref) to a local variable. Args: VN's of: 0: local's number, 1: offset. ValueNumFuncDef(PtrToArrElem, 4, false, false, false, false) // Pointer (byref) to an array element. Args: 0: array elem type eq class var_types value, VN's of: 1: array, 2: index, 3: offset. ValueNumFuncDef(PtrToStatic, 3, false, true, false, false) // Pointer (byref) to a static variable (or possibly a field thereof, if the static variable is a struct). From c9724be83aa4fb3c2994e4b9dac86520d9e17e7f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jul 2023 16:12:24 +0200 Subject: [PATCH 4/6] Remove more dead code --- src/coreclr/jit/valuenum.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index ee8ab34fb3fbf7..8417579ea2ff7a 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1842,7 +1842,6 @@ class ValueNumStore SRC_Null, SRC_Void, SRC_EmptyExcSet, - SRC_EmptyMemDepSet, SRC_NumSpecialRefConsts }; From ab5fc660cc11a7b88d1db937ee5a063dc524bc58 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jul 2023 16:18:23 +0200 Subject: [PATCH 5/6] Nit --- 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 1f87880934aef8..89d2b05fc6314b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2922,7 +2922,9 @@ void ValueNumStore::MapSelectWorkCacheEntry::SetMemoryDependencies(CompAllocator } for (unsigned i = 0; i < m_numMemoryDependencies; i++) - arr[i] = deps.Bottom(startIndex + i); + { + arr[i] = deps.Bottom(startIndex + i); + } } //------------------------------------------------------------------------------ From 66f64d0b50e25c75abf1ee3b4c2f8381d5b0f2eb Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jul 2023 19:24:16 +0200 Subject: [PATCH 6/6] Fix using map selects from other phases --- src/coreclr/jit/valuenum.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 89d2b05fc6314b..45b89527d28b95 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2882,8 +2882,11 @@ ValueNum ValueNumStore::VNForMapSelectInner(ValueNumKind vnk, var_types type, Va // The remaining budget should always be between [0..m_mapSelectBudget] assert((budget >= 0) && (budget <= m_mapSelectBudget)); - // If the current tree is in a loop then record memory dependencies for hoisting. - if (m_pComp->compCurBB->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) + // If the current tree is in a loop then record memory dependencies for + // hoisting. Note that this function may be called by other phases than VN + // (such as VN-based dead store removal). + if ((m_pComp->compCurBB != nullptr) && (m_pComp->compCurTree != nullptr) && + m_pComp->compCurBB->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) { for (int i = 0; i < memoryDependencies.Height(); i++) {