From 97ef6a6e93b6dc5cd7d55ffc121657e588f3c3cb Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 24 Feb 2026 07:57:02 -0800 Subject: [PATCH 1/2] Fix bugs and typos in objectalloc.cpp, extract AnalyzePseudoForCloning Bug fixes: - Fix tree/parent mismatch in AnalyzeParentStack: check parent->OperIs(GT_STORE_BLK) instead of tree->OperIs(GT_STORE_BLK) to match the parent->AsBlk() cast - Fix break exiting entire for loop in AnalyzeIfCloningCanPreventEscape, preventing later pseudos from being processed and marked as escaping - Fix missing printf argument in RecordAppearance JITDUMP for multiple defs - Fix missing format specifiers in JITDUMP calls for escape-independence messages - Fix stale lclNumIndex variable usage in alloc temp escape check - Fix wrong function name in MorphAllocObjNodeHelperArr comment header Refactoring: - Extract loop body of AnalyzeIfCloningCanPreventEscape into new AnalyzePseudoForCloning method with early returns for clarity Typo fixes in comments: - MarIndex -> MarkIndex, indicies -> indices, space space -> space, objectd -> object, pseduo -> pseudo, candiate -> candidate, othr -> other, reasonble -> reasonable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/objectalloc.cpp | 235 +++++++++++++++++--------------- src/coreclr/jit/objectalloc.h | 1 + 2 files changed, 124 insertions(+), 112 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 43b6d798bda784..dfdd2e25a4b4b8 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -314,7 +314,7 @@ void ObjectAllocator::MarkLclVarAsDefinitelyStackPointing(unsigned int lclNum) } //------------------------------------------------------------------------------ -// MarIndexAsDefinitelyStackPointing : Mark resource as definitely pointing +// MarkIndexAsDefinitelyStackPointing : Mark resource as definitely pointing // to a stack-allocated object. // // @@ -360,7 +360,7 @@ void ObjectAllocator::AddConnGraphEdgeIndex(unsigned int sourceBvIndex, unsigned // void ObjectAllocator::PrepareAnalysis() { - // Determine how locals map to indicies in the bit vectors / connection graph. + // Determine how locals map to indices in the bit vectors / connection graph. // // In "lcl num" space // @@ -386,13 +386,13 @@ void ObjectAllocator::PrepareAnalysis() // We reserve the singleton [N+2M] for the "unknown source" local // // LocalToIndex translates from "lcl num" space to "bv" space - // IndexToLocal translates from "bv" space space to "lcl num" space + // IndexToLocal translates from "bv" space to "lcl num" space // const unsigned localCount = m_compiler->lvaCount; unsigned bvNext = 0; // Enumerate which locals are going to appear in our connection - // graph, and assign them BV indicies. + // graph, and assign them BV indices. // for (unsigned lclNum = 0; lclNum < localCount; lclNum++) { @@ -1456,7 +1456,7 @@ bool ObjectAllocator::MorphAllocObjNodeHelper(AllocationCandidate& candidate) } //------------------------------------------------------------------------ -// MorphAllocObjNodeHelperObj: See if we can stack allocate a GT_NEWARR +// MorphAllocObjNodeHelperArr: See if we can stack allocate a GT_NEWARR // // Arguments: // candidate -- allocation candidate @@ -1751,7 +1751,7 @@ unsigned int ObjectAllocator::MorphNewArrNodeIntoStackAlloc(GenTreeCall* // allocation. // Arguments: // allocObj - GT_ALLOCOBJ that will be replaced by a stack allocation -// layout - layout for the stack allocated objectd +// layout - layout for the stack allocated object // block - a basic block where allocObj is // stmt - a statement where allocObj is // @@ -2029,7 +2029,7 @@ void ObjectAllocator::AnalyzeParentStack(ArrayStack* parentStack, unsi break; } - if (tree->OperIs(GT_STORE_BLK)) + if (parent->OperIs(GT_STORE_BLK)) { ClassLayout* const layout = parent->AsBlk()->GetLayout(); @@ -2946,7 +2946,7 @@ void ObjectAllocator::RewriteUses() // // Notes: // During our analysis we have may have noted conditionally escaping objects -// and var references and connected them to a pseduo, along with information +// and var references and connected them to a pseudo, along with information // about how we could clone blocks to ensure that the object could be stack allocated. // // The current assumption is that these nodes do not escape, but to ensure @@ -2956,7 +2956,7 @@ void ObjectAllocator::RewriteUses() // as escaping. If any pseudo now escapes, we return true so that the main // analysis can update its closure. // -// We may choose not to clone a candiate for several reasons: +// We may choose not to clone a candidate for several reasons: // * too much EH already in the method, or some other reason cloning is infeasible // * two different candidates have overlapping clone regions // * the cost/benefit analysis does not look favorable @@ -2970,40 +2970,72 @@ bool ObjectAllocator::AnalyzeIfCloningCanPreventEscape(BitVecTraits* bitVecTrait for (unsigned p = 0; p < m_numPseudos; p++) { unsigned const pseudoIndex = p + m_firstPseudoIndex; - bool canClone = true; - CloneInfo* info = nullptr; - const bool hasInfo = m_CloneMap.Lookup(pseudoIndex, &info); - if (!hasInfo) + if (AnalyzePseudoForCloning(bitVecTraits, escapingNodes, pseudoIndex)) { - // We never found any conditional allocation attached to this pseudoIndex. - // - JITDUMPEXEC(DumpIndex(pseudoIndex)); - JITDUMP(" has no guard info\n"); - canClone = false; - break; + m_regionsToClone++; } - - // See what locals were "assigned" to the pseudo. - // - BitVec pseudoAdjacencies = m_ConnGraphAdjacencyMatrix[pseudoIndex]; - - // If we found an allocation but didn't find any conditionally escaping uses, then cloning is of no use - // - if (BitVecOps::IsEmpty(bitVecTraits, pseudoAdjacencies)) + else { - JITDUMP(" No conditionally escaping uses under"); + JITDUMP(" not optimizing, so will mark"); JITDUMPEXEC(DumpIndex(pseudoIndex)); - JITDUMP(", so no reason to clone\n"); - canClone = false; - break; + JITDUMP(" as escaping\n"); + MarkIndexAsEscaping(pseudoIndex); + BitVecOps::AddElemD(bitVecTraits, escapingNodesToProcess, pseudoIndex); + newEscapes = true; } + } + + return newEscapes; +} + +//------------------------------------------------------------------------------ +// AnalyzePseudoForCloning: analyze a single pseudo to see if cloning can +// prevent the associated object from escaping. +// +// Arguments: +// bitVecTraits - Bit vector traits +// escapingNodes - current set of escaping nodes +// pseudoIndex - index of the pseudo to analyze +// +// Returns: +// true if cloning will proceed for this pseudo +// +bool ObjectAllocator::AnalyzePseudoForCloning(BitVecTraits* bitVecTraits, + BitVec& escapingNodes, + unsigned pseudoIndex) +{ + CloneInfo* info = nullptr; - // Check if each conditionally escaping local escapes on its own; if so cloning is of no use + if (!m_CloneMap.Lookup(pseudoIndex, &info)) + { + // We never found any conditional allocation attached to this pseudoIndex. // + JITDUMPEXEC(DumpIndex(pseudoIndex)); + JITDUMP(" has no guard info\n"); + return false; + } + + // See what locals were "assigned" to the pseudo. + // + BitVec pseudoAdjacencies = m_ConnGraphAdjacencyMatrix[pseudoIndex]; + + // If we found an allocation but didn't find any conditionally escaping uses, then cloning is of no use + // + if (BitVecOps::IsEmpty(bitVecTraits, pseudoAdjacencies)) + { + JITDUMP(" No conditionally escaping uses under"); + JITDUMPEXEC(DumpIndex(pseudoIndex)); + JITDUMP(", so no reason to clone\n"); + return false; + } + + // Check if each conditionally escaping local escapes on its own; if so cloning is of no use + // + { BitVecOps::Iter iterator(bitVecTraits, pseudoAdjacencies); unsigned lclNumIndex = BAD_VAR_NUM; - while (canClone && iterator.NextElem(&lclNumIndex)) + while (iterator.NextElem(&lclNumIndex)) { if (BitVecOps::IsMember(bitVecTraits, escapingNodes, lclNumIndex)) { @@ -3011,101 +3043,80 @@ bool ObjectAllocator::AnalyzeIfCloningCanPreventEscape(BitVecTraits* bitVecTrait // not under a failing GDV or any GDV. // JITDUMPEXEC(DumpIndex(lclNumIndex)); - JITDUMP(" escapes independently of", IndexToLocal(lclNumIndex)); + JITDUMP(" escapes independently of"); JITDUMPEXEC(DumpIndex(pseudoIndex)); JITDUMP("\n"); - canClone = false; - break; - } - } - - // Also check the alloc temps - // - if (canClone && (info->m_allocTemps != nullptr)) - { - for (unsigned v : *(info->m_allocTemps)) - { - if (BitVecOps::IsMember(bitVecTraits, escapingNodes, LocalToIndex(v))) - { - JITDUMP(" alloc temp"); - JITDUMPEXEC(DumpIndex(v)); - JITDUMP(" escapes independently of", IndexToLocal(lclNumIndex)); - JITDUMPEXEC(DumpIndex(pseudoIndex)); - JITDUMP("\n"); - - canClone = false; - break; - } + return false; } } + } - if (canClone) + // Also check the alloc temps + // + if (info->m_allocTemps != nullptr) + { + for (unsigned v : *(info->m_allocTemps)) { - // We may be able to clone and specialize the enumerator uses to ensure - // that the allocated enumerator does not escape. - // - JITDUMPEXEC(DumpIndex(pseudoIndex)); - JITDUMP(" is guarding the escape of V%02u\n", info->m_local); - if (info->m_allocTemps != nullptr) + if (BitVecOps::IsMember(bitVecTraits, escapingNodes, LocalToIndex(v))) { - JITDUMP(" along with "); - for (unsigned v : *(info->m_allocTemps)) - { - JITDUMP("V%02u ", v); - } + JITDUMP(" alloc temp"); + JITDUMPEXEC(DumpIndex(LocalToIndex(v))); + JITDUMP(" escapes independently of"); + JITDUMPEXEC(DumpIndex(pseudoIndex)); JITDUMP("\n"); + return false; } - JITDUMP(" they escape only when V%02u.Type NE %s\n", info->m_local, - m_compiler->eeGetClassName(info->m_type)); - JITDUMP(" V%02u + secondary vars have %u appearances\n", info->m_local, info->m_appearanceCount); - - m_compiler->Metrics.EnumeratorGDVProvisionalNoEscape++; } + } - // See if cloning is actually viable. - // - if (canClone) + // We may be able to clone and specialize the enumerator uses to ensure + // that the allocated enumerator does not escape. + // + JITDUMPEXEC(DumpIndex(pseudoIndex)); + JITDUMP(" is guarding the escape of V%02u\n", info->m_local); + if (info->m_allocTemps != nullptr) + { + JITDUMP(" along with "); + for (unsigned v : *(info->m_allocTemps)) { - canClone = CanClone(info); + JITDUMP("V%02u ", v); } + JITDUMP("\n"); + } + JITDUMP(" they escape only when V%02u.Type NE %s\n", info->m_local, + m_compiler->eeGetClassName(info->m_type)); + JITDUMP(" V%02u + secondary vars have %u appearances\n", info->m_local, info->m_appearanceCount); - // See if this clone would overlap with othr clones - // - if (canClone) - { - canClone = !CloneOverlaps(info); - } + m_compiler->Metrics.EnumeratorGDVProvisionalNoEscape++; - // See if cloning is a good idea. - // - if (canClone) - { - canClone = ShouldClone(info); - } + // See if cloning is actually viable. + // + if (!CanClone(info)) + { + return false; + } - // All checks are done - // - if (canClone) - { - JITDUMP("\n*** Can prevent escape under"); - JITDUMPEXEC(DumpIndex(pseudoIndex)); - JITDUMP(" via cloning ***\n"); + // See if this clone would overlap with other clones + // + if (CloneOverlaps(info)) + { + return false; + } - info->m_willClone = true; - m_regionsToClone++; - } - else - { - JITDUMP(" not optimizing, so will mark"); - JITDUMPEXEC(DumpIndex(pseudoIndex)); - JITDUMP(" as escaping\n"); - MarkIndexAsEscaping(pseudoIndex); - BitVecOps::AddElemD(bitVecTraits, escapingNodesToProcess, pseudoIndex); - newEscapes = true; - } + // See if cloning is a good idea. + // + if (!ShouldClone(info)) + { + return false; } - return newEscapes; + JITDUMP("\n*** Can prevent escape under"); + JITDUMPEXEC(DumpIndex(pseudoIndex)); + JITDUMP(" via cloning ***\n"); + + info->m_willClone = true; + + return true; } //------------------------------------------------------------------------------ @@ -3642,7 +3653,7 @@ void ObjectAllocator::RecordAppearance(unsigned lclNum, BasicBlock* block, State { if (!v->m_hasMultipleDefs) { - JITDUMP("Enumerator V%02u has multiple defs\n"); + JITDUMP("Enumerator V%02u has multiple defs\n", lclNum); v->m_hasMultipleDefs = true; } } @@ -3803,7 +3814,7 @@ bool ObjectAllocator::ShouldClone(CloneInfo* info) //------------------------------------------------------------------------------ // CanClone: check that cloning can remove all escaping references and -// is a reasonble thing to do +// is a reasonable thing to do // // Arguments: // info -- [in, out] info about the cloning opportunity @@ -3826,7 +3837,7 @@ bool ObjectAllocator::CanClone(CloneInfo* info) //------------------------------------------------------------------------------ // CheckCanClone: check that cloning can remove all escaping references and -// is a reasonble thing to do +// is a reasonable thing to do // // Arguments: // info -- [in, out] info about the cloning opportunity diff --git a/src/coreclr/jit/objectalloc.h b/src/coreclr/jit/objectalloc.h index 11d37b9402a5b7..4f8e6fc20c1748 100644 --- a/src/coreclr/jit/objectalloc.h +++ b/src/coreclr/jit/objectalloc.h @@ -281,6 +281,7 @@ class ObjectAllocator final : public Phase bool AnalyzeIfCloningCanPreventEscape(BitVecTraits* bitVecTraits, BitVec& escapingNodes, BitVec& escapingNodesToProcess); + bool AnalyzePseudoForCloning(BitVecTraits* bitVecTraits, BitVec& escapingNodes, unsigned pseudoIndex); bool CanClone(CloneInfo* info); bool CheckCanClone(CloneInfo* info); bool CloneOverlaps(CloneInfo* info); From 28c09b7a3c5f03970c089f619f8b90cc51dab47c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 24 Feb 2026 11:05:03 -0800 Subject: [PATCH 2/2] format --- src/coreclr/jit/objectalloc.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index dfdd2e25a4b4b8..894ed2de75b952 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -3001,9 +3001,7 @@ bool ObjectAllocator::AnalyzeIfCloningCanPreventEscape(BitVecTraits* bitVecTrait // Returns: // true if cloning will proceed for this pseudo // -bool ObjectAllocator::AnalyzePseudoForCloning(BitVecTraits* bitVecTraits, - BitVec& escapingNodes, - unsigned pseudoIndex) +bool ObjectAllocator::AnalyzePseudoForCloning(BitVecTraits* bitVecTraits, BitVec& escapingNodes, unsigned pseudoIndex) { CloneInfo* info = nullptr; @@ -3083,8 +3081,7 @@ bool ObjectAllocator::AnalyzePseudoForCloning(BitVecTraits* bitVecTraits, } JITDUMP("\n"); } - JITDUMP(" they escape only when V%02u.Type NE %s\n", info->m_local, - m_compiler->eeGetClassName(info->m_type)); + JITDUMP(" they escape only when V%02u.Type NE %s\n", info->m_local, m_compiler->eeGetClassName(info->m_type)); JITDUMP(" V%02u + secondary vars have %u appearances\n", info->m_local, info->m_appearanceCount); m_compiler->Metrics.EnumeratorGDVProvisionalNoEscape++;