From 9d4ba69d3cfb5493297a55cce79686dd29a7b020 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 14 Jul 2024 17:43:41 +0200 Subject: [PATCH 1/2] JIT: Change VN's representation for phi definitions Replace `VNF_PhiDef` and `VNF_MemoryPhiDef` by new explicit representations that represent all phi args directly. --- src/coreclr/jit/optimizer.cpp | 31 +- src/coreclr/jit/redundantbranchopts.cpp | 14 +- src/coreclr/jit/valuenum.cpp | 534 ++++++++++++++---------- src/coreclr/jit/valuenum.h | 39 +- src/coreclr/jit/valuenumfuncs.h | 3 - 5 files changed, 366 insertions(+), 255 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 8fb5ca0493566..b2f76f8e02b33 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5162,24 +5162,13 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNS return previousRes; } - bool res = true; - VNFuncApp funcApp; + bool res = true; + VNFuncApp funcApp; + VNPhiDef phiDef; + VNMemoryPhiDef memoryPhiDef; if (vnStore->GetVNFunc(vn, &funcApp)) { - if (funcApp.m_func == VNF_PhiDef) - { - // Is the definition within the loop? If so, is not loop-invariant. - unsigned lclNum = funcApp.m_args[0]; - unsigned ssaNum = funcApp.m_args[1]; - LclSsaVarDsc* ssaDef = lvaTable[lclNum].GetPerSsaData(ssaNum); - res = !loop->ContainsBlock(ssaDef->GetBlock()); - } - else if (funcApp.m_func == VNF_PhiMemoryDef) - { - BasicBlock* defnBlk = reinterpret_cast(vnStore->ConstantValue(funcApp.m_args[0])); - res = !loop->ContainsBlock(defnBlk); - } - else if (funcApp.m_func == VNF_MemOpaque) + if (funcApp.m_func == VNF_MemOpaque) { const unsigned loopIndex = funcApp.m_args[0]; @@ -5239,6 +5228,16 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNS } } } + else if (vnStore->GetPhiDef(vn, &phiDef)) + { + // Is the definition within the loop? If so, is not loop-invariant. + LclSsaVarDsc* ssaDef = lvaTable[phiDef.LclNum].GetPerSsaData(phiDef.SsaDef); + res = !loop->ContainsBlock(ssaDef->GetBlock()); + } + else if (vnStore->GetMemoryPhiDef(vn, &memoryPhiDef)) + { + res = !loop->ContainsBlock(memoryPhiDef.Block); + } loopVnInvariantCache->Set(vn, res); return res; diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index b12ab304c0857..f074fab8032f6 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1398,8 +1398,8 @@ bool Compiler::optJumpThreadPhi(BasicBlock* block, GenTree* tree, ValueNum treeN for (int i = 0; i < 2; i++) { const ValueNum phiDefVN = treeNormVNFuncApp.m_args[i]; - VNFuncApp phiDefFuncApp; - if (!vnStore->GetVNFunc(phiDefVN, &phiDefFuncApp) || (phiDefFuncApp.m_func != VNF_PhiDef)) + VNPhiDef phiDef; + if (!vnStore->GetPhiDef(phiDefVN, &phiDef)) { // This input is not a phi def. If it's a func app it might depend on // transitively on a phi def; consider a general search utility. @@ -1409,12 +1409,10 @@ bool Compiler::optJumpThreadPhi(BasicBlock* block, GenTree* tree, ValueNum treeN // The PhiDef args tell us which local and which SSA def of that local. // - assert(phiDefFuncApp.m_arity == 3); - const unsigned lclNum = unsigned(phiDefFuncApp.m_args[0]); - const unsigned ssaDefNum = unsigned(phiDefFuncApp.m_args[1]); - const ValueNum phiVN = ValueNum(phiDefFuncApp.m_args[2]); - JITDUMP("... JT-PHI [interestingVN] in " FMT_BB " relop %s operand VN is PhiDef for V%02u:%u " FMT_VN "\n", - block->bbNum, i == 0 ? "first" : "second", lclNum, ssaDefNum, phiVN); + const unsigned lclNum = phiDef.LclNum; + const unsigned ssaDefNum = phiDef.SsaDef; + JITDUMP("... JT-PHI [interestingVN] in " FMT_BB " relop %s operand VN is PhiDef for V%02u\n", block->bbNum, + i == 0 ? "first" : "second", lclNum, ssaDefNum); if (!foundPhiDef) { DISPTREE(tree); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 7e00b61aabb25..ecd4678c4b629 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -1733,6 +1733,14 @@ ValueNumStore::Chunk::Chunk(CompAllocator alloc, ValueNum* pNextBaseVN, var_type m_defs = new (alloc) VNHandle[ChunkSize]; break; + case CEA_PhiDef: + m_defs = new (alloc) VNPhiDef[ChunkSize]; + break; + + case CEA_MemoryPhiDef: + m_defs = new (alloc) VNMemoryPhiDef[ChunkSize]; + break; + case CEA_Func0: m_defs = new (alloc) VNFunc[ChunkSize]; break; @@ -2860,8 +2868,7 @@ ValueNum ValueNumStore::VNForFuncNoFolding(var_types typ, VNFunc func, ValueNum // // Return Value: - Returns the ValueNum associated with 'func'('arg0VN','arg1VN','arg1VN) // -// Note: - This method only handles Trinary operations -// We have to special case VNF_PhiDef, as it's first two arguments are not ValueNums +// Note: - This method only handles ternary operations // ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, ValueNum arg1VN, ValueNum arg2VN) { @@ -2874,15 +2881,8 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V #ifdef DEBUG // Function arguments carry no exceptions. // - if (func != VNF_PhiDef) - { - // For a phi definition first and second argument are "plain" local/ssa numbers. - // (I don't know if having such non-VN arguments to a VN function is a good idea -- if we wanted to declare - // ValueNum to be "short" it would be a problem, for example. But we'll leave it for now, with these explicit - // exceptions.) - assert(arg0VN == VNNormalValue(arg0VN)); - assert(arg1VN == VNNormalValue(arg1VN)); - } + assert(arg0VN == VNNormalValue(arg0VN)); + assert(arg1VN == VNNormalValue(arg1VN)); assert(arg2VN == VNNormalValue(arg2VN)); #endif @@ -2958,6 +2958,123 @@ ValueNum ValueNumStore::VNForFunc( return *resultVN; } +// ---------------------------------------------------------------------------------------- +// VNForPhiDef - Return a new VN number for a phi definition. +// +// Arguments: +// type - Type of the local +// lclNum - Local corresponding to the phidef +// ssaDef - SSA number representing the phi def +// ssaArgs - SSA numbers from the predecessors +// +// Return Value: +// New value number +// +ValueNum ValueNumStore::VNForPhiDef(var_types type, unsigned lclNum, unsigned ssaDef, ArrayStack& ssaArgs) +{ + unsigned* newSsaArgs = m_alloc.allocate((unsigned)ssaArgs.Height()); + memcpy(newSsaArgs, ssaArgs.Data(), (unsigned)ssaArgs.Height() * sizeof(ValueNum)); + + Chunk* const c = GetAllocChunk(type, CEA_PhiDef); + unsigned index = c->AllocVN(); + VNPhiDef* inserted = &static_cast(c->m_defs)[index]; + + inserted->LclNum = lclNum; + inserted->SsaDef = ssaDef; + inserted->SsaArgs = newSsaArgs; + inserted->NumArgs = (unsigned)ssaArgs.Height(); + + ValueNum newVN = c->m_baseVN + index; + return newVN; +} + +// ---------------------------------------------------------------------------------------- +// GetPhiDef - Check if a VN represents a phi definition and if so, look up +// information about it. +// +// Arguments: +// vn - Value number +// phiDef - [out] Information about the phi definition +// +// Return Value: +// True if the VN is a phi def. +// +bool ValueNumStore::GetPhiDef(ValueNum vn, VNPhiDef* phiDef) +{ + if (vn == NoVN) + { + return false; + } + + Chunk* c = m_chunks.GetNoExpand(GetChunkNum(vn)); + unsigned offset = ChunkOffset(vn); + assert(offset < c->m_numUsed); + if (c->m_attribs == CEA_PhiDef) + { + *phiDef = static_cast(c->m_defs)[offset]; + return true; + } + + return false; +} + +// ---------------------------------------------------------------------------------------- +// VNForMemoryPhiDef - Return a new VN number for a memory phi definition. +// +// Arguments: +// block - Block with the definition +// ssaArgs - SSA numbers from the predecessors +// +// Return Value: +// New value number +// +ValueNum ValueNumStore::VNForMemoryPhiDef(BasicBlock* block, ArrayStack& ssaArgs) +{ + unsigned* newSsaArgs = m_alloc.allocate((unsigned)ssaArgs.Height()); + memcpy(newSsaArgs, ssaArgs.Data(), (unsigned)ssaArgs.Height() * sizeof(ValueNum)); + + Chunk* const c = GetAllocChunk(TYP_HEAP, CEA_MemoryPhiDef); + unsigned index = c->AllocVN(); + VNMemoryPhiDef* inserted = &static_cast(c->m_defs)[index]; + + inserted->Block = block; + inserted->SsaArgs = newSsaArgs; + inserted->NumArgs = (unsigned)ssaArgs.Height(); + + ValueNum newVN = c->m_baseVN + index; + return newVN; +} + +// ---------------------------------------------------------------------------------------- +// GetMemoryPhiDef - Check if a VN represents a memory phi definition and if +// so, look up information about it. +// +// Arguments: +// vn - Value number +// phiDef - [out] Information about the memory phi definition +// +// Return Value: +// True if the VN is a memory phi def. +// +bool ValueNumStore::GetMemoryPhiDef(ValueNum vn, VNMemoryPhiDef* memoryPhiDef) +{ + if (vn == NoVN) + { + return false; + } + + Chunk* c = m_chunks.GetNoExpand(GetChunkNum(vn)); + unsigned offset = ChunkOffset(vn); + assert(offset < c->m_numUsed); + if (c->m_attribs == CEA_MemoryPhiDef) + { + *memoryPhiDef = static_cast(c->m_defs)[offset]; + return true; + } + + return false; +} + //------------------------------------------------------------------------------ // VNForMapStore: Create the VN for a precise store (to a precise map). // @@ -3440,141 +3557,110 @@ ValueNum ValueNumStore::VNForMapSelectWork(ValueNumKind vnk, } break; - case VNF_PhiDef: - case VNF_PhiMemoryDef: + default: + break; + } + } + else + { + unsigned* ssaArgs = nullptr; + unsigned numSsaArgs = 0; + unsigned lclNum = BAD_VAR_NUM; + + VNPhiDef phiDef; + VNMemoryPhiDef memoryPhiDef; + if (GetPhiDef(map, &phiDef)) + { + lclNum = phiDef.LclNum; + ssaArgs = phiDef.SsaArgs; + numSsaArgs = phiDef.NumArgs; + } + else if (GetMemoryPhiDef(map, &memoryPhiDef)) + { + ssaArgs = memoryPhiDef.SsaArgs; + numSsaArgs = memoryPhiDef.NumArgs; + } + + if (ssaArgs != nullptr) + { + // 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)); + + ValueNum sameSelResult = ValueNumStore::RecursiveVN; + bool allSame = true; + + for (unsigned i = 0; i < numSsaArgs; i++) { - unsigned lclNum = BAD_VAR_NUM; - bool isMemory = false; - VNFuncApp phiFuncApp; - bool defArgIsFunc = false; - if (funcApp.m_func == VNF_PhiDef) + if (*pBudget <= 0) + { + allSame = false; + break; + } + + ValueNum phiArgVN; + if (lclNum != BAD_VAR_NUM) { - lclNum = unsigned(funcApp.m_args[0]); - defArgIsFunc = GetVNFunc(funcApp.m_args[2], &phiFuncApp); + phiArgVN = m_pComp->lvaGetDesc(lclNum)->GetPerSsaData(ssaArgs[i])->m_vnPair.Get(vnk); } else { - assert(funcApp.m_func == VNF_PhiMemoryDef); - isMemory = true; - defArgIsFunc = GetVNFunc(funcApp.m_args[1], &phiFuncApp); + phiArgVN = m_pComp->GetMemoryPerSsaData(ssaArgs[i])->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. + allSame = false; + break; + } - // We need to be careful about breaking infinite recursion. Record the outer select. - m_fixedPointMapSels.Push(VNDefFuncApp<2>(VNF_MapSelect, map, index)); + bool usedRecursiveVN = false; + ValueNum curResult = + VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, &usedRecursiveVN, recMemoryDependencies); - assert(IsVNConstant(phiFuncApp.m_args[0])); - unsigned phiArgSsaNum = ConstantValue(phiFuncApp.m_args[0]); - ValueNum phiArgVN; - 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) - { - bool allSame = true; - ValueNum argRest = phiFuncApp.m_args[1]; - ValueNum sameSelResult = VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, - pUsedRecursiveVN, recMemoryDependencies); - - // 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) - { - // We don't have any budget remaining to verify that all phiArgs are the same - // so setup the default failure case now. - allSame = false; - } + *pUsedRecursiveVN |= usedRecursiveVN; + if (sameSelResult == ValueNumStore::RecursiveVN) + { + sameSelResult = curResult; + } + if ((curResult != ValueNumStore::RecursiveVN) && (curResult != sameSelResult)) + { + allSame = false; + break; + } + } - while (allSame && argRest != ValueNumStore::NoVN) - { - 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) - { - allSame = false; - } - else - { - bool usedRecursiveVN = false; - ValueNum curResult = VNForMapSelectWork(vnk, type, phiArgVN, index, pBudget, - &usedRecursiveVN, recMemoryDependencies); + // Make sure we're popping what we pushed. + assert(FixedPointMapSelsTopHasValue(map, index)); + m_fixedPointMapSels.Pop(); - *pUsedRecursiveVN |= usedRecursiveVN; - if (sameSelResult == ValueNumStore::RecursiveVN) - { - sameSelResult = curResult; - } - if (curResult != ValueNumStore::RecursiveVN && curResult != sameSelResult) - { - allSame = false; - } - } - } - 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) - { - entry.Result = sameSelResult; - entry.SetMemoryDependencies(m_pComp, recMemoryDependencies); + if (allSame && (sameSelResult != ValueNumStore::RecursiveVN)) + { + // 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) + { + entry.Result = sameSelResult; + entry.SetMemoryDependencies(m_pComp, recMemoryDependencies); - GetMapSelectWorkCache()->Set(fstruct, entry); - } + GetMapSelectWorkCache()->Set(fstruct, entry); + } - recMemoryDependencies.ForEach([this, &memoryDependencies](ValueNum vn) { - memoryDependencies.Add(m_pComp, vn); - }); + recMemoryDependencies.ForEach([this, &memoryDependencies](ValueNum vn) { + memoryDependencies.Add(m_pComp, vn); + }); - return sameSelResult; - } - // 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(); - } + return sameSelResult; } - break; - - default: - break; } + + // Otherwise, fall through to creating the select(phi(m1, m2), x) function application. } // We may have run out of budget and already assigned a result @@ -6360,7 +6446,8 @@ var_types ValueNumStore::TypeOfVN(ValueNum vn) const // FlowGraphNaturalLoop* ValueNumStore::LoopOfVN(ValueNum vn) { - VNFuncApp funcApp; + VNFuncApp funcApp; + VNMemoryPhiDef memoryPhiDef; if (GetVNFunc(vn, &funcApp)) { if (funcApp.m_func == VNF_MemOpaque) @@ -6383,11 +6470,10 @@ FlowGraphNaturalLoop* ValueNumStore::LoopOfVN(ValueNum vn) return m_pComp->m_loops->GetLoopByIndex(index); } - else if (funcApp.m_func == VNF_PhiMemoryDef) - { - BasicBlock* const block = reinterpret_cast(ConstantValue(funcApp.m_args[0])); - return m_pComp->m_blockToLoop->GetLoop(block); - } + } + else if (GetMemoryPhiDef(vn, &memoryPhiDef)) + { + return m_pComp->m_blockToLoop->GetLoop(memoryPhiDef.Block); } return nullptr; @@ -6718,16 +6804,6 @@ const char* ValueNumStore::VNRelationString(VN_RELATION_KIND vrk) } #endif -bool ValueNumStore::IsVNPhiDef(ValueNum vn) -{ - VNFuncApp funcAttr; - if (!GetVNFunc(vn, &funcAttr)) - { - return false; - } - - return funcAttr.m_func == VNF_PhiDef; -} //------------------------------------------------------------------------ // AreVNsEquivalent: returns true iff VNs represent the same value // @@ -6748,24 +6824,14 @@ bool ValueNumStore::AreVNsEquivalent(ValueNum vn1, ValueNum vn2) return true; } - VNFuncApp funcAttr1; - if (!GetVNFunc(vn1, &funcAttr1)) - { - return false; - } - - if (funcAttr1.m_func != VNF_PhiDef) + VNPhiDef def1; + if (!GetPhiDef(vn1, &def1)) { return false; } - VNFuncApp funcAttr2; - if (!GetVNFunc(vn2, &funcAttr2)) - { - return false; - } - - if (funcAttr2.m_func != VNF_PhiDef) + VNPhiDef def2; + if (!GetPhiDef(vn2, &def2)) { return false; } @@ -6773,16 +6839,16 @@ bool ValueNumStore::AreVNsEquivalent(ValueNum vn1, ValueNum vn2) // We have two PhiDefs. They may be equivalent, if // they come from Phis in the same block. // - const unsigned lclNum1 = unsigned(funcAttr1.m_args[0]); - const unsigned ssaDefNum1 = unsigned(funcAttr1.m_args[1]); + const unsigned lclNum1 = def1.LclNum; + const unsigned ssaDefNum1 = def1.SsaDef; LclVarDsc* const varDsc1 = m_pComp->lvaGetDesc(lclNum1); LclSsaVarDsc* const varSsaDsc1 = varDsc1->GetPerSsaData(ssaDefNum1); GenTree* const varDefTree1 = varSsaDsc1->GetDefNode(); BasicBlock* const varDefBlock1 = varSsaDsc1->GetBlock(); - const unsigned lclNum2 = unsigned(funcAttr2.m_args[0]); - const unsigned ssaDefNum2 = unsigned(funcAttr2.m_args[1]); + const unsigned lclNum2 = def2.LclNum; + const unsigned ssaDefNum2 = def2.SsaDef; LclVarDsc* const varDsc2 = m_pComp->lvaGetDesc(lclNum2); LclSsaVarDsc* const varSsaDsc2 = varDsc2->GetPerSsaData(ssaDefNum2); @@ -6810,6 +6876,8 @@ bool ValueNumStore::AreVNsEquivalent(ValueNum vn1, ValueNum vn2) bool phiArgsAreEquivalent = true; + // TODO-CQ: This logic could walk the SSA nums in the VNPhiDef, which + // accounts for unreachable predecessors. for (; (treeIter1 != treeEnd1) && (treeIter2 != treeEnd2); ++treeIter1, ++treeIter2) { GenTreePhiArg* const treePhiArg1 = treeIter1->GetNode()->AsPhiArg(); @@ -9819,6 +9887,8 @@ bool ValueNumStore::VNIsValid(ValueNum vn) void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) { printf(" {"); + VNPhiDef phiDef; + VNMemoryPhiDef memoryPhiDef; if (vn == NoVN) { printf("NoVN"); @@ -10058,6 +10128,24 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) printf(")"); } } + else if (GetPhiDef(vn, &phiDef)) + { + printf("PhiDef(V%02u d:%u", phiDef.LclNum, phiDef.SsaDef); + for (unsigned i = 0; i < phiDef.NumArgs; i++) + { + printf(", u:%u", phiDef.SsaArgs[i]); + } + printf(")"); + } + else if (GetMemoryPhiDef(vn, &memoryPhiDef)) + { + printf("MemoryPhiDef(" FMT_BB, memoryPhiDef.Block->bbNum); + for (unsigned i = 0; i < memoryPhiDef.NumArgs; i++) + { + printf(", m:%u", memoryPhiDef.SsaArgs[i]); + } + printf(")"); + } else { // Otherwise, just a VN with no structure; print just the VN. @@ -10905,10 +10993,32 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) // First: visit phis and check to see if all phi args have the same value. for (; (stmt != nullptr) && stmt->IsPhiDefnStmt(); stmt = stmt->GetNextStmt()) { - GenTreeLclVar* newSsaDef = stmt->GetRootNode()->AsLclVar(); - fgValueNumberPhiDef(newSsaDef, blk); +#ifdef DEBUG + if (verbose) + { + printf("\n***** " FMT_BB ", " FMT_STMT "(before)\n", blk->bbNum, stmt->GetID()); + gtDispTree(stmt->GetRootNode()); + printf("\n"); + } +#endif + + fgValueNumberPhiDef(stmt->GetRootNode()->AsLclVar(), blk); + +#ifdef DEBUG + if (verbose) + { + printf("\n***** " FMT_BB ", " FMT_STMT "(after)\n", blk->bbNum, stmt->GetID()); + gtDispTree(stmt->GetRootNode()); + printf("\n"); + if (stmt->GetNextStmt() != nullptr) + { + printf("---------\n"); + } + } +#endif } + ArrayStack phiArgSsaNums(getAllocator(CMK_ValueNumber)); // Now do the same for each MemoryKind. for (MemoryKind memoryKind : allMemoryKinds()) { @@ -10942,41 +11052,37 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) // There should be > 1 args to a phi. // But OSR might leave around "dead" try entry blocks... assert((phiArgs->m_nextArg != nullptr) || opts.IsOSR()); - ValueNum phiAppVN = vnStore->VNForIntCon(phiArgs->GetSsaNum()); - JITDUMP(" Building phi application: $%x = SSA# %d.\n", phiAppVN, phiArgs->GetSsaNum()); - bool allSame = true; - ValueNum sameVN = GetMemoryPerSsaData(phiArgs->GetSsaNum())->m_vnPair.GetLiberal(); - if (sameVN == ValueNumStore::NoVN) - { - allSame = false; - } - phiArgs = phiArgs->m_nextArg; + phiArgSsaNums.Reset(); + + JITDUMP(" Building memory phi def for block " FMT_BB ".\n", blk->bbNum); + ValueNum sameVN = ValueNumStore::NoVN; + while (phiArgs != nullptr) { ValueNum phiArgVN = GetMemoryPerSsaData(phiArgs->GetSsaNum())->m_vnPair.GetLiberal(); - if (phiArgVN == ValueNumStore::NoVN || phiArgVN != sameVN) + + if (phiArgSsaNums.Height() == 0) { - allSame = false; + sameVN = phiArgVN; } -#ifdef DEBUG - ValueNum oldPhiAppVN = phiAppVN; -#endif - unsigned phiArgSSANum = phiArgs->GetSsaNum(); - ValueNum phiArgSSANumVN = vnStore->VNForIntCon(phiArgSSANum); - JITDUMP(" Building phi application: $%x = SSA# %d.\n", phiArgSSANumVN, phiArgSSANum); - phiAppVN = vnStore->VNForFuncNoFolding(TYP_HEAP, VNF_Phi, phiArgSSANumVN, phiAppVN); - JITDUMP(" Building phi application: $%x = phi($%x, $%x).\n", phiAppVN, phiArgSSANumVN, - oldPhiAppVN); + else + { + if ((phiArgVN == ValueNumStore::NoVN) || (phiArgVN != sameVN)) + { + sameVN = ValueNumStore::NoVN; + } + } + + phiArgSsaNums.Push(phiArgs->GetSsaNum()); phiArgs = phiArgs->m_nextArg; } - if (allSame) + if (sameVN != ValueNumStore::NoVN) { newMemoryVN = sameVN; } else { - newMemoryVN = vnStore->VNForFuncNoFolding(TYP_HEAP, VNF_PhiMemoryDef, - vnStore->VNForHandle(ssize_t(blk), GTF_EMPTY), phiAppVN); + newMemoryVN = vnStore->VNForMemoryPhiDef(blk, phiArgSsaNums); } } GetMemoryPerSsaData(blk->bbMemorySsaNumIn[memoryKind])->m_vnPair.SetLiberal(newMemoryVN); @@ -11054,7 +11160,7 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) } //------------------------------------------------------------------------ -// fgValueNumberRegisterConstFieldSeq: If a VN'd integer constant has a +// fgValueNumberPhiDef: If a VN'd integer constant has a // field sequence we want to keep track of, then register it in the side table. // // Arguments: @@ -11064,8 +11170,9 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk) // void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bool isUpdate) { + ArrayStack phiArgSsaNums(getAllocator(CMK_ValueNumber)); + GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); - ValueNumPair phiVNP; ValueNumPair sameVNP; for (GenTreePhi::Use& use : phiNode->Uses()) @@ -11076,7 +11183,7 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo JITDUMP(" Phi arg [%06u] is unnecessary; path through pred " FMT_BB " cannot be taken\n", dspTreeID(phiArg), phiArg->gtPredBB->bbNum); - if ((use.GetNext() != nullptr) || (phiVNP.GetLiberal() != ValueNumStore::NoVN)) + if ((use.GetNext() != nullptr) || (phiArgSsaNums.Height() > 0)) { continue; } @@ -11085,31 +11192,28 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo JITDUMP(" ..but no other path can, so we are using it anyway\n"); } - ValueNum phiArgSsaNumVN = vnStore->VNForIntCon(phiArg->GetSsaNum()); - ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; + ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; - if (isUpdate && (phiArgVNP != phiArg->gtVNPair)) +#ifdef DEBUG + if (verbose && isUpdate && (phiArgVNP != phiArg->gtVNPair)) { - JITDUMP("Updating phi arg [%06u] VN from ", dspTreeID(phiArg)); - JITDUMPEXEC(vnpPrint(phiArg->gtVNPair, 0)); - JITDUMP(" to "); - JITDUMPEXEC(vnpPrint(phiArgVNP, 0)); - JITDUMP("\n"); + printf("Updating phi arg [%06u] VN from ", dspTreeID(phiArg)); + vnpPrint(phiArg->gtVNPair, 0); + printf(" to "); + vnpPrint(phiArgVNP, 0); + printf("\n"); } +#endif phiArg->gtVNPair = phiArgVNP; - if (phiVNP.GetLiberal() == ValueNumStore::NoVN) + if (phiArgSsaNums.Height() == 0) { // This is the first PHI argument - phiVNP = ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN); sameVNP = phiArgVNP; } else { - phiVNP = vnStore->VNPairForFuncNoFolding(newSsaDef->TypeGet(), VNF_Phi, - ValueNumPair(phiArgSsaNumVN, phiArgSsaNumVN), phiVNP); - if ((sameVNP.GetLiberal() != phiArgVNP.GetLiberal()) || (sameVNP.GetConservative() != phiArgVNP.GetConservative())) { @@ -11119,13 +11223,12 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo sameVNP.SetBoth(ValueNumStore::NoVN); } } - } - // We should have visited at least one phi arg in the loop above - assert(phiVNP.GetLiberal() != ValueNumStore::NoVN); - assert(phiVNP.GetConservative() != ValueNumStore::NoVN); + phiArgSsaNums.Push(phiArg->GetSsaNum()); + } - ValueNumPair newSsaDefVNP; + LclSsaVarDsc* newSsaDefDsc = lvaGetDesc(newSsaDef)->GetPerSsaData(newSsaDef->GetSsaNum()); + ValueNumPair newSsaDefVNP = newSsaDefDsc->m_vnPair; if (sameVNP.BothDefined()) { @@ -11133,18 +11236,13 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo // a reason to have the phi -- just pass on that value. newSsaDefVNP = sameVNP; } - else + else if (!isUpdate) { // They were not the same; we need to create a phi definition. - ValueNum lclNumVN = ValueNum(newSsaDef->GetLclNum()); - ValueNum ssaNumVN = ValueNum(newSsaDef->GetSsaNum()); - - newSsaDefVNP = vnStore->VNPairForFunc(newSsaDef->TypeGet(), VNF_PhiDef, ValueNumPair(lclNumVN, lclNumVN), - ValueNumPair(ssaNumVN, ssaNumVN), phiVNP); + newSsaDefVNP.SetBoth( + vnStore->VNForPhiDef(newSsaDef->TypeGet(), newSsaDef->GetLclNum(), newSsaDef->GetSsaNum(), phiArgSsaNums)); } - LclSsaVarDsc* newSsaDefDsc = lvaGetDesc(newSsaDef)->GetPerSsaData(newSsaDef->GetSsaNum()); - #ifdef DEBUG if (isUpdate) { diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index b9592d8c8cae2..52466152439a9 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -206,6 +206,21 @@ struct VNFuncApp } }; +struct VNPhiDef +{ + unsigned LclNum; + unsigned SsaDef; + unsigned* SsaArgs; + unsigned NumArgs; +}; + +struct VNMemoryPhiDef +{ + BasicBlock* Block; + unsigned* SsaArgs; + unsigned NumArgs; +}; + // We use a unique prefix character when printing value numbers in dumps: i.e. $1c0 // This define is used with string concatenation to put this in printf format strings #define FMT_VN "$%x" @@ -700,6 +715,11 @@ class ValueNumStore // Skip all folding checks. ValueNum VNForFuncNoFolding(var_types typ, VNFunc func, ValueNum op1VNwx, ValueNum op2VNwx); + ValueNum VNForPhiDef(var_types type, unsigned lclNum, unsigned ssaDef, ArrayStack& ssaArgs); + bool GetPhiDef(ValueNum vn, VNPhiDef* phiDef); + ValueNum VNForMemoryPhiDef(BasicBlock* block, ArrayStack& vns); + bool GetMemoryPhiDef(ValueNum vn, VNMemoryPhiDef* memoryPhiDef); + ValueNum VNForCast(VNFunc func, ValueNum castToVN, ValueNum objVN); ValueNum VNForMapSelect(ValueNumKind vnk, var_types type, ValueNum map, ValueNum index); @@ -1068,9 +1088,6 @@ class ValueNumStore // Returns true iff the VN represents a relop bool IsVNRelop(ValueNum vn); - // Returns true iff the VN is a phi definition - bool IsVNPhiDef(ValueNum vn); - // Returns true if the two VNs represent the same value // despite being different VNs. Useful for phi def VNs. bool AreVNsEquivalent(ValueNum vn1, ValueNum vn2); @@ -1398,13 +1415,15 @@ class ValueNumStore enum ChunkExtraAttribs : BYTE { - CEA_Const, // This chunk contains constant values. - CEA_Handle, // This chunk contains handle constants. - CEA_Func0, // Represents functions of arity 0. - CEA_Func1, // ...arity 1. - CEA_Func2, // ...arity 2. - CEA_Func3, // ...arity 3. - CEA_Func4, // ...arity 4. + CEA_Const, // This chunk contains constant values. + CEA_Handle, // This chunk contains handle constants. + CEA_PhiDef, // This contains pointers to VNPhiDef. + CEA_MemoryPhiDef, // This contains pointers to VNMemoryPhiDef. + CEA_Func0, // Represents functions of arity 0. + CEA_Func1, // ...arity 1. + CEA_Func2, // ...arity 2. + CEA_Func3, // ...arity 3. + CEA_Func4, // ...arity 4. CEA_Count }; diff --git a/src/coreclr/jit/valuenumfuncs.h b/src/coreclr/jit/valuenumfuncs.h index 70c950fccfc7e..50cf4c6a274c3 100644 --- a/src/coreclr/jit/valuenumfuncs.h +++ b/src/coreclr/jit/valuenumfuncs.h @@ -12,9 +12,6 @@ ValueNumFuncDef(MapStore, 4, false, false, false, false) // Args: 0: m ValueNumFuncDef(MapPhysicalStore, 3, false, false, false, false) // Args: 0: map, 1: "physical selector": offset and size, 2: value being stored ValueNumFuncDef(BitCast, 2, false, false, false, false) // Args: 0: VN of the arg, 1: VN of the target type ValueNumFuncDef(ZeroObj, 1, false, false, false, false) // Args: 0: VN of the class handle. -ValueNumFuncDef(PhiDef, 3, false, false, false, false) // Args: 0: local var # (or -1 for memory), 1: SSA #, 2: VN of definition. -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(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. From d9b068a1d93b06e86d7986d8862c70924dd11990 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 21 Jul 2024 16:27:50 +0200 Subject: [PATCH 2/2] Make it no-diff --- src/coreclr/jit/valuenum.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index ecd4678c4b629..6b73027bcc968 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11236,11 +11236,30 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo // a reason to have the phi -- just pass on that value. newSsaDefVNP = sameVNP; } - else if (!isUpdate) + else { // They were not the same; we need to create a phi definition. - newSsaDefVNP.SetBoth( - vnStore->VNForPhiDef(newSsaDef->TypeGet(), newSsaDef->GetLclNum(), newSsaDef->GetSsaNum(), phiArgSsaNums)); + + bool newPhiDef = true; + if (isUpdate) + { + // For an update, the phi-def usually does not have any new + // information -- it is still just the list of SSA args from preds. + // The exception is if we were now able to prove that the block is + // unreachable from one of the preds. + VNPhiDef prevPhiDef; + if (vnStore->GetPhiDef(newSsaDefDsc->m_vnPair.GetLiberal(), &prevPhiDef) && + (prevPhiDef.NumArgs == (unsigned)phiArgSsaNums.Height())) + { + newPhiDef = false; + } + } + + if (newPhiDef) + { + newSsaDefVNP.SetBoth(vnStore->VNForPhiDef(newSsaDef->TypeGet(), newSsaDef->GetLclNum(), + newSsaDef->GetSsaNum(), phiArgSsaNums)); + } } #ifdef DEBUG