From 812df7bd7c0a5004a749b99ad480e1060deeface Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 27 Mar 2025 09:59:59 -0700 Subject: [PATCH 01/13] JIT: enhance escape analysis to understand local struct fields Continuation of #113141 Implement a very simplistic "field sensitive" analysis for gc structs where we model each struct as simply its gc field(s). That is, given a gc struct `G` with GC field `f`, we model ``` G.f = ... ``` as an assignment to `G` and ``` = G.f ``` as a read from `G`. Since we know `G` itself cannot escape, "escaping" of `G` means `G.f` can escape. Note this conflates the fields of `G`: either they all escape or none of them do. We could make this more granular but it's not clear that doing so is worthwhile, and it requires more up-front work to determine how to track each field's status. If the struct or struct fields are only consumed locally, we may be able to prove the gc referents don't escape. This is a subset/elaboration of #112543 that does not try and reason interprocedurally. Contributes to #104936 / #108913 Fixes #113236 (once the right inlines happen) --- src/coreclr/jit/compiler.h | 13 +- src/coreclr/jit/jitconfigvalues.h | 1 + src/coreclr/jit/layout.cpp | 215 +++++++++++- src/coreclr/jit/layout.h | 5 +- src/coreclr/jit/objectalloc.cpp | 213 +++++++++-- src/coreclr/jit/objectalloc.h | 3 + .../ObjectStackAllocationTests.cs | 331 +++++++++++++++++- 7 files changed, 715 insertions(+), 66 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4571efd26adb0b..ffa794e8243b01 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1036,6 +1036,13 @@ class LclVarDsc m_layout = layout; } + // Change the layout to one that may not be compatible. + void ChangeLayout(ClassLayout* layout) + { + assert(varTypeIsStruct(lvType)); + m_layout = layout; + } + // Grow the size of a block layout local. void GrowBlockLayout(ClassLayout* layout) { @@ -10928,8 +10935,10 @@ class Compiler unsigned typGetBlkLayoutNum(unsigned blockSize); // Get the layout for the specified array of known length ClassLayout* typGetArrayLayout(CORINFO_CLASS_HANDLE classHandle, unsigned length); - // Get the number of a layout for the specified array of known length - unsigned typGetArrayLayoutNum(CORINFO_CLASS_HANDLE classHandle, unsigned length); + // Get a layout like an existing layout, with all gc refs removed + ClassLayout* typGetNonGCLayout(ClassLayout* existingLayout); + // Get a layout like an existing layout, with all gc refs changed to byrefs + ClassLayout* typGetByrefLayout(ClassLayout* existingLayout); var_types TypeHandleToVarType(CORINFO_CLASS_HANDLE handle, ClassLayout** pLayout = nullptr); var_types TypeHandleToVarType(CorInfoType jitType, CORINFO_CLASS_HANDLE handle, ClassLayout** pLayout = nullptr); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 6988d11debe332..927ba9ebdfa5ac 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -678,6 +678,7 @@ RELEASE_CONFIG_INTEGER(JitObjectStackAllocationConditionalEscape, "JitObjectStac CONFIG_STRING(JitObjectStackAllocationConditionalEscapeRange, "JitObjectStackAllocationConditionalEscapeRange") RELEASE_CONFIG_INTEGER(JitObjectStackAllocationArray, "JitObjectStackAllocationArray", 1) RELEASE_CONFIG_INTEGER(JitObjectStackAllocationSize, "JitObjectStackAllocationSize", 528) +RELEASE_CONFIG_INTEGER(JitObjectStackAllocationTrackFields, "JitObjectStackAllocationTrackFields", 1) RELEASE_CONFIG_INTEGER(JitEECallTimingInfo, "JitEECallTimingInfo", 0) diff --git a/src/coreclr/jit/layout.cpp b/src/coreclr/jit/layout.cpp index bb281674254790..13c5f8774da9e4 100644 --- a/src/coreclr/jit/layout.cpp +++ b/src/coreclr/jit/layout.cpp @@ -414,18 +414,80 @@ ClassLayout* Compiler::typGetBlkLayout(unsigned blockSize) return typGetCustomLayout(ClassLayoutBuilder(this, blockSize)); } -unsigned Compiler::typGetArrayLayoutNum(CORINFO_CLASS_HANDLE classHandle, unsigned length) +ClassLayout* Compiler::typGetArrayLayout(CORINFO_CLASS_HANDLE classHandle, unsigned length) { ClassLayoutBuilder b = ClassLayoutBuilder::BuildArray(this, classHandle, length); - return typGetCustomLayoutNum(b); + return typGetCustomLayout(b); } -ClassLayout* Compiler::typGetArrayLayout(CORINFO_CLASS_HANDLE classHandle, unsigned length) +ClassLayout* Compiler::typGetNonGCLayout(ClassLayout* layout) { - ClassLayoutBuilder b = ClassLayoutBuilder::BuildArray(this, classHandle, length); + assert(layout->HasGCPtr()); + ClassLayoutBuilder b(this, layout->GetSize()); + b.CopyPaddingFrom(0, layout); + +#ifdef DEBUG + b.CopyNameFrom(layout, "[nongc] "); +#endif + + return typGetCustomLayout(b); +} + +ClassLayout* Compiler::typGetByrefLayout(ClassLayout* layout) +{ + assert(layout->HasGCPtr()); + ClassLayoutBuilder b(this, layout->GetSize()); + b.CopyPaddingFrom(0, layout); + b.CopyGCInfoFromMakeByref(0, layout); + +#ifdef DEBUG + b.CopyNameFrom(layout, "[byref] "); +#endif + return typGetCustomLayout(b); } +#ifdef DEBUG +//------------------------------------------------------------------------ +// CopyNameFrom: Copy layout names, with optional prefix. +// +// Parameters: +// layout - layout to copy from +// prefix - prefix to add (or nullptr) +// +void ClassLayoutBuilder::CopyNameFrom(ClassLayout* layout, const char* prefix) +{ + const char* layoutName = layout->GetClassName(); + const char* layoutShortName = layout->GetShortClassName(); + + if (prefix != nullptr) + { + char* newName = nullptr; + char* newShortName = nullptr; + + if (layoutName != nullptr) + { + size_t len = strlen(prefix) + strlen(layoutName) + 1; + newName = m_compiler->getAllocator(CMK_DebugOnly).allocate(len); + sprintf_s(newName, len, "%s%s", prefix, layoutShortName); + } + + if (layoutShortName != nullptr) + { + size_t len = strlen(prefix) + strlen(layoutName) + 1; + newShortName = m_compiler->getAllocator(CMK_DebugOnly).allocate(len); + sprintf_s(newShortName, len, "%s%s", prefix, layoutShortName); + } + + SetName(newName, newShortName); + } + else + { + SetName(layoutName, layoutShortName); + } +} +#endif // DEBUG + //------------------------------------------------------------------------ // Create: Create a ClassLayout from an EE side class handle. // @@ -646,8 +708,8 @@ const SegmentList& ClassLayout::GetNonPadding(Compiler* comp) // AreCompatible: check if 2 layouts are the same for copying. // // Arguments: -// layout1 - the first layout; -// layout2 - the second layout. +// layout1 - the first layout (copy destination) +// layout2 - the second layout (copy source) // // Return value: // true if compatible, false otherwise. @@ -658,6 +720,8 @@ const SegmentList& ClassLayout::GetNonPadding(Compiler* comp) // // This is an equivalence relation: // AreCompatible(a, b) == AreCompatible(b, a) +// AreCompatible(a, a) == true +// AreCompatible(a, b) && AreCompatible(b, c) ==> AreCompatible(a, c) // // static bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* layout2) @@ -746,9 +810,92 @@ bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* l // bool ClassLayout::CanAssignFrom(const ClassLayout* layout) { - // Currently this is the same as compatability + if (this == layout) + { + return true; + } + + // Do the normal compatibility check first, when possible to do so. + // + if ((IsCustomLayout() == layout->IsCustomLayout()) || (!HasGCPtr() && !layout->HasGCPtr())) + { + const bool areCompatible = AreCompatible(this, layout); + + if (areCompatible) + { + return true; + } + } + + // Must be same size + // + if (GetSize() != layout->GetSize()) + { + return false; + } + + // Must be same IR type + // + if (GetType() != layout->GetType()) + { + return false; + } + + // Dest is GC, source is GC. Allow, slotwise: // - return AreCompatible(this, layout); + // byref <- ref, byref, nint + // ref <- ref + // nint <- nint + // + if (HasGCPtr() && layout->HasGCPtr()) + { + const unsigned slotsCount = GetSlotCount(); + assert(slotsCount == layout->GetSlotCount()); + + for (unsigned i = 0; i < slotsCount; ++i) + { + var_types slotType = GetGCPtrType(i); + var_types layoutSlotType = layout->GetGCPtrType(i); + + if ((slotType != TYP_BYREF) && (slotType != layoutSlotType)) + { + return false; + } + } + return true; + } + + // Dest is GC, source is noGC. Allow, slotwise: + // + // byref <- nint + // nint <- nint + // + if (HasGCPtr() && !layout->HasGCPtr()) + { + const unsigned slotsCount = GetSlotCount(); + + for (unsigned i = 0; i < slotsCount; ++i) + { + var_types slotType = GetGCPtrType(i); + if (slotType == TYP_REF) + { + return false; + } + } + return true; + } + + // Dest is noGC, source is GC. Disallow. + // + if (!HasGCPtr() && layout->HasGCPtr()) + { + assert(!HasGCPtr()); + return false; + } + + // Dest is noGC, source is noGC, and they're not compatible. + // + return false; } //------------------------------------------------------------------------ @@ -814,7 +961,7 @@ ClassLayoutBuilder ClassLayoutBuilder::BuildArray(Compiler* compiler, CORINFO_CL unsigned offset = OFFSETOF__CORINFO_Array__data; for (unsigned i = 0; i < length; i++) { - builder.CopyInfoFrom(offset, elementLayout, /* copy padding */ false); + builder.CopyGCInfoFrom(offset, elementLayout); offset += elementSize; } } @@ -919,14 +1066,13 @@ void ClassLayoutBuilder::SetGCPtrType(unsigned slot, var_types type) } //------------------------------------------------------------------------ -// CopyInfoFrom: Copy GC pointers and padding information from another layout. +// CopyInfoGCFrom: Copy GC pointers from another layout. // // Arguments: // offset - Offset in this builder to start copy information into. // layout - Layout to get information from. -// copyPadding - Whether padding info should also be copied from the layout. // -void ClassLayoutBuilder::CopyInfoFrom(unsigned offset, ClassLayout* layout, bool copyPadding) +void ClassLayoutBuilder::CopyGCInfoFrom(unsigned offset, ClassLayout* layout) { assert(offset + layout->GetSize() <= m_size); @@ -939,18 +1085,53 @@ void ClassLayoutBuilder::CopyInfoFrom(unsigned offset, ClassLayout* layout, bool SetGCPtr(startSlot + slot, layout->GetGCPtr(slot)); } } +} - if (copyPadding) - { - AddPadding(SegmentList::Segment(offset, offset + layout->GetSize())); +//------------------------------------------------------------------------ +// CopyInfoGCFromMakeByref: Copy GC pointers from another layout,and change +// all gc references to be TYP_BYREF (TYPE_GC_BYREF) +// +// Arguments: +// offset - Offset in this builder to start copy information into. +// layout - Layout to get information from. +// +void ClassLayoutBuilder::CopyGCInfoFromMakeByref(unsigned offset, ClassLayout* layout) +{ + assert(offset + layout->GetSize() <= m_size); - for (const SegmentList::Segment& nonPadding : layout->GetNonPadding(m_compiler)) + if (layout->GetGCPtrCount() > 0) + { + assert(offset % TARGET_POINTER_SIZE == 0); + unsigned startSlot = offset / TARGET_POINTER_SIZE; + for (unsigned slot = 0; slot < layout->GetSlotCount(); slot++) { - RemovePadding(SegmentList::Segment(offset + nonPadding.Start, offset + nonPadding.End)); + CorInfoGCType gcType = layout->GetGCPtr(slot); + if (gcType == TYPE_GC_REF) + { + gcType = TYPE_GC_BYREF; + } + SetGCPtr(startSlot + slot, gcType); } } } +//------------------------------------------------------------------------ +// CopyInfoPaddingFrom: Copy padding from another layout. +// +// Arguments: +// offset - Offset in this builder to start copy information into. +// layout - Layout to get information from. +// +void ClassLayoutBuilder::CopyPaddingFrom(unsigned offset, ClassLayout* layout) +{ + AddPadding(SegmentList::Segment(offset, offset + layout->GetSize())); + + for (const SegmentList::Segment& nonPadding : layout->GetNonPadding(m_compiler)) + { + RemovePadding(SegmentList::Segment(offset + nonPadding.Start, offset + nonPadding.End)); + } +} + //------------------------------------------------------------------------ // GetOrCreateNonPadding: Get the non padding segment list, or create it if it // does not exist. diff --git a/src/coreclr/jit/layout.h b/src/coreclr/jit/layout.h index e92fee4ff43553..dee61727e115e5 100644 --- a/src/coreclr/jit/layout.h +++ b/src/coreclr/jit/layout.h @@ -34,12 +34,15 @@ class ClassLayoutBuilder ClassLayoutBuilder(Compiler* compiler, unsigned size); void SetGCPtrType(unsigned slot, var_types type); - void CopyInfoFrom(unsigned offset, ClassLayout* layout, bool copyPadding); + void CopyGCInfoFrom(unsigned offset, ClassLayout* layout); + void CopyGCInfoFromMakeByref(unsigned offset, ClassLayout* layout); + void CopyPaddingFrom(unsigned offset, ClassLayout* layout); void AddPadding(const SegmentList::Segment& padding); void RemovePadding(const SegmentList::Segment& nonPadding); #ifdef DEBUG void SetName(const char* name, const char* shortName); + void CopyNameFrom(ClassLayout* layout, const char* prefix); #endif static ClassLayoutBuilder BuildArray(Compiler* compiler, CORINFO_CLASS_HANDLE arrayType, unsigned length); diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index a86197994e377d..824b30d133f41a 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -52,12 +52,14 @@ ObjectAllocator::ObjectAllocator(Compiler* comp) , m_numPseudoLocals(0) , m_maxPseudoLocals(0) , m_regionsToClone(0) + , m_trackFields(false) { m_EscapingPointers = BitVecOps::UninitVal(); m_PossiblyStackPointingPointers = BitVecOps::UninitVal(); m_DefinitelyStackPointingPointers = BitVecOps::UninitVal(); m_ConnGraphAdjacencyMatrix = nullptr; m_StackAllocMaxSize = (unsigned)JitConfig.JitObjectStackAllocationSize(); + m_trackFields = JitConfig.JitObjectStackAllocationTrackFields() > 0; } //------------------------------------------------------------------------ @@ -72,7 +74,9 @@ ObjectAllocator::ObjectAllocator(Compiler* comp) bool ObjectAllocator::IsTrackedType(var_types type) { const bool isTrackableScalar = (type == TYP_REF) || (genActualType(type) == TYP_I_IMPL) || (type == TYP_BYREF); - return isTrackableScalar; + const bool isTrackableStruct = (type == TYP_STRUCT) && m_trackFields; + + return isTrackableScalar || isTrackableStruct; } //------------------------------------------------------------------------ @@ -476,6 +480,7 @@ void ObjectAllocator::PrepareAnalysis() } JITDUMP("%u locals, %u tracked by escape analysis\n", localCount, m_nextLocalIndex); + JITDUMP("Local field tracking is %s\n", m_trackFields ? "enabled" : "disabled"); if (m_nextLocalIndex > 0) { @@ -592,7 +597,7 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() lclEscapes = false; m_allocator->CheckForGuardedAllocationOrCopy(m_block, m_stmt, use, lclNum); } - else if (tree->OperIs(GT_LCL_VAR) && tree->TypeIs(TYP_REF, TYP_BYREF, TYP_I_IMPL)) + else if (tree->OperIs(GT_LCL_VAR, GT_LCL_ADDR) && m_allocator->IsTrackedLocal(lclNum)) { assert(tree == m_ancestors.Top()); if (!m_allocator->CanLclVarEscapeViaParentStack(&m_ancestors, lclNum, m_block)) @@ -668,6 +673,11 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() JITDUMP(" V%02u is address exposed\n", lclNum); MarkLclVarAsEscaping(lclNum); } + else if (lclNum == comp->info.compRetBuffArg) + { + JITDUMP(" V%02u is retbuff\n", lclNum); + MarkLclVarAsEscaping(lclNum); + } // Parameters have unknown initial values. // OSR locals have unknown initial values. @@ -1554,10 +1564,13 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent assert(parentStack != nullptr); int parentIndex = 1; - bool keepChecking = true; - bool canLclVarEscapeViaParentStack = true; - bool isCopy = true; - bool isEnumeratorLocal = comp->lvaGetDesc(lclNum)->lvIsEnumerator; + LclVarDsc* const lclDsc = comp->lvaGetDesc(lclNum); + + bool keepChecking = true; + bool canLclVarEscapeViaParentStack = true; + bool isCopy = true; + bool const isEnumeratorLocal = lclDsc->lvIsEnumerator; + int numIndirs = 0; while (keepChecking) { @@ -1580,10 +1593,11 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent switch (parent->OperGet()) { // Update the connection graph if we are storing to a local. - // For all other stores we mark the local as escaping. + // case GT_STORE_LCL_VAR: { - // Add an edge to the connection graph. + // Add an edge to the connection graph, if destination is a local we're tracking + // (i.e. a local var or a field of a local var). const unsigned int dstLclNum = parent->AsLclVar()->GetLclNum(); const unsigned int srcLclNum = lclNum; @@ -1612,6 +1626,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent case GT_GE: case GT_NULLCHECK: case GT_ARR_LENGTH: + case GT_BOUNDS_CHECK: canLclVarEscapeViaParentStack = false; break; @@ -1626,9 +1641,22 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent case GT_COLON: case GT_QMARK: case GT_ADD: - case GT_SUB: case GT_FIELD_ADDR: - // Check whether the local escapes via its grandparent. + case GT_LCL_ADDR: + // Check whether the local escapes higher up + ++parentIndex; + keepChecking = true; + break; + + case GT_SUB: + // Sub of two GC refs is no longer a GC ref. + if (!parent->TypeIs(TYP_BYREF, TYP_REF)) + { + canLclVarEscapeViaParentStack = false; + break; + } + + // Check whether the local escapes higher up ++parentIndex; keepChecking = true; break; @@ -1654,33 +1682,106 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent case GT_STOREIND: case GT_STORE_BLK: case GT_BLK: - if (tree != parent->AsIndir()->Addr()) + { + GenTree* const addr = parent->AsIndir()->Addr(); + if (tree != addr) { - // TODO-ObjectStackAllocation: track stores to fields. - break; + JITDUMP("... store value\n"); + + // Is this a store to a field of a local struct...? + // + if (parent->OperIs(GT_STOREIND) && addr->OperIs(GT_FIELD_ADDR, GT_LCL_ADDR)) + { + // Do we know which local? + // + GenTree* const base = addr->OperIs(GT_FIELD_ADDR) ? addr->AsOp()->gtGetOp1() : addr; + + if (base->OperIs(GT_LCL_ADDR)) + { + unsigned const dstLclNum = base->AsLclVarCommon()->GetLclNum(); + + if (IsTrackedLocal(dstLclNum)) + { + JITDUMP("... local [struct] store\n"); + // Add an edge to the connection graph. + AddConnGraphEdge(dstLclNum, lclNum); + canLclVarEscapeViaParentStack = false; + } + else + { + // Store to untracked local. Assume escape. + } + } + else + { + // Store destination unknown. Assume escape. + // + // TODO: handle more general trees here. + // Since we visit the address subtree first, perhaps we can annotate somehow. + } + } + else + { + // Likely heap store. Assume escape. + } } - FALLTHROUGH; + else + { + canLclVarEscapeViaParentStack = false; + JITDUMP("... store address\n"); + } + break; + } + case GT_IND: - // Address of the field/ind is not taken so the local doesn't escape. + { + GenTree* const addr = parent->AsIndir()->Addr(); + + // For loads from structs we may be tracking the underlying fields. + // + // We don't handle TYP_REF locals (yet), and allowing that requires separating out the object from + // its fields in our tracking. + // + // We treat TYP_BYREF like TYP_STRUCT, though possibly this needs more scrutiny, as byrefs may alias. + // Ditto for TYP_I_IMPL. + // + // We can assume that the local being read is lclNum, since we have walked up to this node from a leaf + // local. + // + // We only track through the first indir. + // + if (m_trackFields && (numIndirs == 0) && varTypeIsGC(parent->TypeGet()) && + (lclDsc->TypeGet() != TYP_REF)) + { + JITDUMP("... local [struct] load\n"); + ++parentIndex; + ++numIndirs; + keepChecking = true; + break; + } + + // Address doesn't refer to anything we track + // canLclVarEscapeViaParentStack = false; break; + } case GT_CALL: { - GenTreeCall* const asCall = parent->AsCall(); + GenTreeCall* const call = parent->AsCall(); - if (asCall->IsHelperCall()) + if (call->IsHelperCall()) { canLclVarEscapeViaParentStack = - !Compiler::s_helperCallProperties.IsNoEscape(comp->eeGetHelperNum(asCall->gtCallMethHnd)); + !Compiler::s_helperCallProperties.IsNoEscape(comp->eeGetHelperNum(call->gtCallMethHnd)); } - else if (asCall->IsSpecialIntrinsic()) + else if (call->IsSpecialIntrinsic()) { // Some known special intrinsics don't escape. At this moment, only the ones accepting byrefs // are supported. In order to support more intrinsics accepting objects, we need extra work // on the VM side which is not ready for that yet. // - switch (comp->lookupNamedIntrinsic(asCall->gtCallMethHnd)) + switch (comp->lookupNamedIntrinsic(call->gtCallMethHnd)) { case NI_System_SpanHelpers_ClearWithoutReferences: case NI_System_SpanHelpers_Fill: @@ -1787,9 +1888,9 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p FALLTHROUGH; case GT_QMARK: case GT_ADD: - case GT_SUB: case GT_FIELD_ADDR: case GT_INDEX_ADDR: + case GT_LCL_ADDR: case GT_BOX: if (parent->TypeGet() != newType) { @@ -1799,6 +1900,15 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p keepChecking = true; break; + case GT_SUB: + if (parent->TypeGet() != newType) + { + parent->ChangeType(newType); + ++parentIndex; + keepChecking = true; + } + break; + case GT_COLON: { GenTree* const lhs = parent->AsOp()->gtGetOp1(); @@ -1828,17 +1938,18 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p case GT_STOREIND: case GT_STORE_BLK: case GT_BLK: - assert(tree == parent->AsIndir()->Addr()); - - // The new target could be *not* on the heap. - parent->gtFlags &= ~GTF_IND_TGT_HEAP; - - if (newType != TYP_BYREF) + if (tree == parent->AsIndir()->Addr()) { - // This indicates that a write barrier is not needed when writing - // to this field/indirection since the address is not pointing to the heap. - // It's either null or points to inside a stack-allocated object. - parent->gtFlags |= GTF_IND_TGT_NOT_HEAP; + // The new target could be *not* on the heap. + parent->gtFlags &= ~GTF_IND_TGT_HEAP; + + if (newType != TYP_BYREF) + { + // This indicates that a write barrier is not needed when writing + // to this field/indirection since the address is not pointing to the heap. + // It's either null or points to inside a stack-allocated object. + parent->gtFlags |= GTF_IND_TGT_NOT_HEAP; + } } break; @@ -1847,6 +1958,8 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p break; default: + JITDUMP("UpdateAncestorTypes: unexpected op %s in [%06u]\n", GenTree::OpName(parent->OperGet()), + comp->dspTreeID(parent)); unreached(); } @@ -1898,10 +2011,7 @@ void ObjectAllocator::RewriteUses() if (m_allocator->MayLclVarPointToStack(lclNum)) { - // Analysis does not handle indirect access to pointer locals. - assert(tree->OperIsScalarLocal()); - - var_types newType; + var_types newType = TYP_UNDEF; if (m_allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum)) { assert(tree->OperIs(GT_LCL_VAR)); // Must be a use. @@ -1912,15 +2022,44 @@ void ObjectAllocator::RewriteUses() else { newType = m_allocator->DoesLclVarPointToStack(lclNum) ? TYP_I_IMPL : TYP_BYREF; - tree->ChangeType(newType); + if (!tree->TypeIs(TYP_STRUCT)) + { + tree->ChangeType(newType); + } } - if (lclVarDsc->lvType != newType) + // For local structs, retype the GC fields. + // + if (lclVarDsc->lvType == TYP_STRUCT) + { + ClassLayout* const layout = lclVarDsc->GetLayout(); + ClassLayout* newLayout = nullptr; + + if ((newType == TYP_I_IMPL) && !layout->IsBlockLayout()) + { + // New layout with no gc refs + padding + newLayout = m_compiler->typGetNonGCLayout(layout); + JITDUMP("Changing layout of struct V%02u to block\n", lclNum); + lclVarDsc->ChangeLayout(newLayout); + } + else if (!layout->IsCustomLayout()) // hacky... want to know if there are any TYP_GC + { + // New layout with all gc refs as byrefs + padding + // (todo, perhaps: see if old layout was already all byrefs) + newLayout = m_compiler->typGetByrefLayout(layout); + JITDUMP("Changing layout of struct V%02u to byref\n", lclNum); + lclVarDsc->ChangeLayout(newLayout); + } + } + // For locals, retype the local + // + else if (lclVarDsc->lvType != newType) { JITDUMP("Changing the type of V%02u from %s to %s\n", lclNum, varTypeName(lclVarDsc->lvType), varTypeName(newType)); lclVarDsc->lvType = newType; } + m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType); if (newLclNum != BAD_VAR_NUM) diff --git a/src/coreclr/jit/objectalloc.h b/src/coreclr/jit/objectalloc.h index 161fe79a7a5e2f..e06fd943b95146 100644 --- a/src/coreclr/jit/objectalloc.h +++ b/src/coreclr/jit/objectalloc.h @@ -149,6 +149,9 @@ class ObjectAllocator final : public Phase unsigned m_maxPseudoLocals; unsigned m_regionsToClone; + // Struct fields + bool m_trackFields; + //=============================================================================== // Methods public: diff --git a/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs b/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs index 6065b9cb3ae7a5..2bdb6c1d407035 100644 --- a/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs +++ b/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs @@ -81,6 +81,19 @@ struct NestedStruct public SimpleStruct s; } + struct GCStruct + { + public int i; + public int[] o1; + public int[] o2; + } + + struct GCStruct2 + { + public string[] a; + public string[] b; + } + enum AllocationKind { Heap, @@ -88,6 +101,13 @@ enum AllocationKind Undefined } + ref struct SpanKeeper + { + public int a; + public Span span; + public int b; + } + public class Tests { static volatile int f1 = 5; @@ -108,7 +128,8 @@ public class Tests public static int TestEntryPoint() { AllocationKind expectedAllocationKind = AllocationKind.Stack; - if (GCStressEnabled()) { + if (GCStressEnabled()) + { Console.WriteLine("GCStress is enabled"); expectedAllocationKind = AllocationKind.Undefined; } @@ -162,8 +183,18 @@ public static int TestEntryPoint() CallTestAndVerifyAllocation(AllocateArrayT, 84, expectedAllocationKind); CallTestAndVerifyAllocation(AllocateArrayT, 84, expectedAllocationKind); + // Spans + CallTestAndVerifyAllocation(SpanCaptureArray1, 41, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanCaptureArray2, 25, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanCaptureArrayT, 37, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanCaptureArrayT, 37, expectedAllocationKind); + + // Other structs with GC fields. + CallTestAndVerifyAllocation(StructReferredObjects, 25, expectedAllocationKind); + // The remaining tests currently never allocate on the stack - if (expectedAllocationKind == AllocationKind.Stack) { + if (expectedAllocationKind == AllocationKind.Stack) + { expectedAllocationKind = AllocationKind.Heap; } @@ -176,6 +207,19 @@ public static int TestEntryPoint() CallTestAndVerifyAllocation(AllocateArrayWithNonGCElementsEscape, 42, expectedAllocationKind); CallTestAndVerifyAllocation(AllocateArrayWithGCElementsEscape, 42, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanEscapeArrayArg, 42, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanEscapeArrayArgCopy, 42, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanEscapeArrayOutParam, 22, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanEscapeArrayOutParam2, 22, expectedAllocationKind); + CallTestAndVerifyAllocation(SpanEscapeRef, 55, expectedAllocationKind); + + // Structs + CallTestAndVerifyAllocation(StructReferredObjectEscape1, 33, expectedAllocationKind); + CallTestAndVerifyAllocation(StructReferredObjectEscape2, 33, expectedAllocationKind); + CallTestAndVerifyAllocation(StructReferredObjectEscape3, 41, expectedAllocationKind); + CallTestAndVerifyAllocation(StructReferredObjectEscape4, 41, expectedAllocationKind); + CallTestAndVerifyAllocation(StructReferredObjectEscape5, 5, expectedAllocationKind); + // This test calls CORINFO_HELP_OVERFLOW CallTestAndVerifyAllocation(AllocateArrayWithNonGCElementsOutOfRangeLeft, 0, expectedAllocationKind, true); @@ -205,27 +249,34 @@ static void CallTestAndVerifyAllocation(Test test, int expectedResult, Allocatio int testResult = test(); long allocatedBytesAfter = GC.GetAllocatedBytesForCurrentThread(); - if (testResult != expectedResult) { + if (testResult != expectedResult) + { Console.WriteLine($"FAILURE ({methodName}): expected {expectedResult}, got {testResult}"); methodResult = -1; } - else if ((expectedAllocationsKind == AllocationKind.Stack) && (allocatedBytesBefore != allocatedBytesAfter)) { + else if ((expectedAllocationsKind == AllocationKind.Stack) && (allocatedBytesBefore != allocatedBytesAfter)) + { Console.WriteLine($"FAILURE ({methodName}): unexpected allocation of {allocatedBytesAfter - allocatedBytesBefore} bytes"); methodResult = -1; } - else if ((expectedAllocationsKind == AllocationKind.Heap) && (allocatedBytesBefore == allocatedBytesAfter)) { + else if ((expectedAllocationsKind == AllocationKind.Heap) && (allocatedBytesBefore == allocatedBytesAfter)) + { Console.WriteLine($"FAILURE ({methodName}): unexpected stack allocation"); methodResult = -1; } - else { + else + { Console.WriteLine($"SUCCESS ({methodName})"); } } - catch { - if (throws) { + catch + { + if (throws) + { Console.WriteLine($"SUCCESS ({methodName})"); } - else { + else + { throw; } } @@ -403,6 +454,123 @@ static int AllocateArrayT() return array.Length + 42; } + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanCaptureArray1() + { + Span span = new int[100]; + span[10] = 41; + return span[10] + span[42]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanCaptureArray2() => SpanCaptureArray2Helper(null); + + static int SpanCaptureArray2Helper(int[]? x) + { + Span span = x ?? new int[100]; + span[10] = 25; + return span[10] + span[42]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanCaptureArray3() + { + Span span = new int[128]; + span[10] = 100; + Span x = span; + return x[10] + span[42]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanCaptureArrayT() + { + Span span = new T[37]; + Use(span[0]); + return span.Length; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanEscapeArrayArg() + { + Span y = new int[100]; + Use(y); + TrashStack(); + return y[42]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanEscapeArrayArgCopy() + { + Span x = new int[100]; + Span y = x; + Use(y); + TrashStack(); + return y[42]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanEscapeArrayReturn() => SpanEscapeArrayReturnHelper()[10]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static Span SpanEscapeArrayReturnHelper() + { + Span x = new int[44]; + Span y = x; + x[10] = 99; + return y; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanEscapeArrayOutParam() + { + Span x; + SpanEscapeArrayOutParamHelper(out x); + TrashStack(); + return x[10]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void SpanEscapeArrayOutParamHelper(out Span a) + { + a = new Span(new int[44]); + a[10] = 22; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanEscapeArrayOutParam2() + { + SpanKeeper y; + SpanEscapeArrayOutParam2Helper(out y); + TrashStack(); + return y.span[10]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void SpanEscapeArrayOutParam2Helper(out SpanKeeper b) + { + int[] x = new int[44]; + x[10] = 22; + b.span = x; + b.a = 1; + b.b = 2; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SpanEscapeRef() + { + ref int q = ref SpanEscapeRef(55); + TrashStack(); + return q; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static ref int SpanEscapeRef(int n) + { + Span x = new int[100]; + x[99] = n; + return ref x[99]; + } + [MethodImpl(MethodImplOptions.NoInlining)] static int AllocateArrayWithNonGCElementsEscape() { @@ -455,6 +623,126 @@ static int AllocateLongLengthArrayWithNonGCElements() return 1; } + [MethodImpl(MethodImplOptions.NoInlining)] + static int StructReferredObjects() + { + int[] a1 = new int[10]; + int[] a2 = new int[10]; + + a1[3] = 7; + a2[4] = 8; + + GCStruct s = new GCStruct() { i = 10, o1 = a1, o2 = a2 }; + + return s.i + s.o1[3] + s.o2[4]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int StructReferredObjectEscape1() => StructReferredObjectEscape1Helper()[3]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int[] StructReferredObjectEscape1Helper() + { + int[] a1 = new int[10]; + int[] a2 = a1; + + a1[3] = 33; + a2[4] = 8; + + GCStruct s = new GCStruct() { i = 10, o1 = a1, o2 = a2 }; + + return s.o2; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int StructReferredObjectEscape2() + { + ref int a = ref StructReferredObjectEscape2Helper(); + TrashStack(); + return a; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static ref int StructReferredObjectEscape2Helper() + { + int[] a1 = new int[10]; + int[] a2 = a1; + + a1[3] = 33; + a2[4] = 8; + + GCStruct s = new GCStruct() { i = 10, o1 = a1, o2 = a2 }; + return ref s.o2[3]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int StructReferredObjectEscape3() + { + GCStruct s = StructReferredObjectEscape3Helper(); + return s.o1[3] + s.o2[4]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static GCStruct StructReferredObjectEscape3Helper() + { + int[] a1 = new int[10]; + int[] a2 = a1; + + a1[3] = 33; + a2[4] = 8; + + GCStruct s = new GCStruct() { i = 10, o1 = a1, o2 = a2 }; + return s; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int StructReferredObjectEscape4() + { + GCStruct s; + StructReferredObjectEscape4Helper(out s); + return s.o1[3] + s.o2[4]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void StructReferredObjectEscape4Helper(out GCStruct s) + { + int[] a1 = new int[10]; + int[] a2 = a1; + + a1[3] = 33; + a2[4] = 8; + + s = new GCStruct() { i = 10, o1 = a1, o2 = a2 }; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static int StructReferredObjectEscape5() + { + string[] s = StructReferredObjectEscape5Helper(0); + TrashStack(); + return s[0].Length; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static string[] StructReferredObjectEscape5Helper(int n) + { + GCStruct2 g = new GCStruct2(); + g.a = new string[10]; + g.b = new string[10]; + + g.a[0] = "Goodbye"; + g.b[0] = "Hello"; + + ref string[] rs = ref g.b; + + if (n > 0) + { + rs = ref g.a; + } + + return rs; + } + [MethodImpl(MethodImplOptions.NoInlining)] static void Use(ref int v) { @@ -467,6 +755,21 @@ static void Use(ref string s) s = "42"; } + [MethodImpl(MethodImplOptions.NoInlining)] + static void Use(Span span) + { + span[42] = 42; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Use(T t) + { + + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static Span Identity(Span x) => x; + [MethodImpl(MethodImplOptions.NoInlining)] private static void ZeroAllocTest() { @@ -526,6 +829,16 @@ private static void Consume(T _) { } + [MethodImpl(MethodImplOptions.NoInlining)] + static void TrashStack() + { + Span span = stackalloc int[128]; + for (int i = 0; i < span.Length; i++) + { + span[i] = -1; + } + } + private record class MyRecord(int A, long B, Guid C); } } From b6bf10b0be99400771c31d3752bfa9eb55bc8a63 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 30 Mar 2025 18:06:01 -0700 Subject: [PATCH 02/13] handle non-struct local indirs better --- src/coreclr/jit/jitconfigvalues.h | 1 + src/coreclr/jit/objectalloc.cpp | 84 ++++++++++++++++++++----------- 2 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 927ba9ebdfa5ac..f28df2144369c9 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -679,6 +679,7 @@ CONFIG_STRING(JitObjectStackAllocationConditionalEscapeRange, "JitObjectStackAll RELEASE_CONFIG_INTEGER(JitObjectStackAllocationArray, "JitObjectStackAllocationArray", 1) RELEASE_CONFIG_INTEGER(JitObjectStackAllocationSize, "JitObjectStackAllocationSize", 528) RELEASE_CONFIG_INTEGER(JitObjectStackAllocationTrackFields, "JitObjectStackAllocationTrackFields", 1) +CONFIG_STRING(JitObjectStackAllocationTrackFieldsRange, "JitObjectStackAllocationTrackFieldsRange") RELEASE_CONFIG_INTEGER(JitEECallTimingInfo, "JitEECallTimingInfo", 0) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 824b30d133f41a..729701f6156fe9 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -441,6 +441,22 @@ void ObjectAllocator::PrepareAnalysis() } } +#ifdef DEBUG + if (m_trackFields) + { + static ConfigMethodRange JitObjectStackAllocationTrackFieldsRange; + JitObjectStackAllocationTrackFieldsRange.EnsureInit(JitConfig.JitObjectStackAllocationTrackFieldsRange()); + const unsigned hash = comp->info.compMethodHash(); + const bool inRange = JitObjectStackAllocationTrackFieldsRange.Contains(hash); + + if (!inRange) + { + JITDUMP("Disabling field wise escape analysis per range config\n"); + m_trackFields = false; + } + } +#endif + // When we clone to prevent conditional escape, we'll also create a new local // var that we will track. So we need to leave room for these vars. There can // be as many of these as there are pseudo locals. @@ -1684,21 +1700,42 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent case GT_BLK: { GenTree* const addr = parent->AsIndir()->Addr(); - if (tree != addr) + if (tree == addr) { - JITDUMP("... store value\n"); + JITDUMP("... store address\n"); + canLclVarEscapeViaParentStack = false; + break; + } - // Is this a store to a field of a local struct...? - // - if (parent->OperIs(GT_STOREIND) && addr->OperIs(GT_FIELD_ADDR, GT_LCL_ADDR)) + JITDUMP("... store value\n"); + + // Is this a store to a field of a local struct...? + // + if (parent->OperIs(GT_STOREIND)) + { + if (addr->OperIs(GT_LCL_ADDR)) + { + unsigned const dstLclNum = addr->AsLclVarCommon()->GetLclNum(); + LclVarDsc* const dstDsc = comp->lvaGetDesc(dstLclNum); + + if (IsTrackedLocal(dstLclNum)) + { + JITDUMP("... local [indir] store\n"); + // Add an edge to the connection graph. + AddConnGraphEdge(dstLclNum, lclNum); + canLclVarEscapeViaParentStack = false; + } + } + else if (addr->OperIs(GT_FIELD_ADDR)) { - // Do we know which local? + // Simple check for which local. // - GenTree* const base = addr->OperIs(GT_FIELD_ADDR) ? addr->AsOp()->gtGetOp1() : addr; + GenTree* const base = addr->AsOp()->gtGetOp1(); if (base->OperIs(GT_LCL_ADDR)) { - unsigned const dstLclNum = base->AsLclVarCommon()->GetLclNum(); + unsigned const dstLclNum = base->AsLclVarCommon()->GetLclNum(); + LclVarDsc* const dstDsc = comp->lvaGetDesc(dstLclNum); if (IsTrackedLocal(dstLclNum)) { @@ -1707,28 +1744,8 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent AddConnGraphEdge(dstLclNum, lclNum); canLclVarEscapeViaParentStack = false; } - else - { - // Store to untracked local. Assume escape. - } - } - else - { - // Store destination unknown. Assume escape. - // - // TODO: handle more general trees here. - // Since we visit the address subtree first, perhaps we can annotate somehow. } } - else - { - // Likely heap store. Assume escape. - } - } - else - { - canLclVarEscapeViaParentStack = false; - JITDUMP("... store address\n"); } break; } @@ -1750,8 +1767,15 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // // We only track through the first indir. // - if (m_trackFields && (numIndirs == 0) && varTypeIsGC(parent->TypeGet()) && - (lclDsc->TypeGet() != TYP_REF)) + if (addr->OperIs(GT_LCL_ADDR)) + { + JITDUMP("... local [indir] load\n"); + ++parentIndex; + ++numIndirs; + keepChecking = true; + break; + } + else if (m_trackFields && (numIndirs == 0)) { JITDUMP("... local [struct] load\n"); ++parentIndex; From 70e2d143e6526f12164961c338ba6776eb972458 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 31 Mar 2025 08:06:09 -0700 Subject: [PATCH 03/13] only consider loads of tracked types --- src/coreclr/jit/objectalloc.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 729701f6156fe9..0b2728d8c7bbd3 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -1752,6 +1752,14 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent case GT_IND: { + // Does this load a type we're tracking? + // + if (!IsTrackedType(parent->TypeGet())) + { + canLclVarEscapeViaParentStack = false; + break; + } + GenTree* const addr = parent->AsIndir()->Addr(); // For loads from structs we may be tracking the underlying fields. @@ -1784,7 +1792,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent break; } - // Address doesn't refer to anything we track + // Address doesn't refer to any location we track // canLclVarEscapeViaParentStack = false; break; From e0432bd0844fe989e43f42ce0b8ab08038fbef03 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 31 Mar 2025 10:40:42 -0700 Subject: [PATCH 04/13] restrict general indirs to byrefs and structs --- src/coreclr/jit/objectalloc.cpp | 20 ++++++++++--------- .../ObjectStackAllocationTests.cs | 10 +++++----- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 0b2728d8c7bbd3..b8aecb028be755 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -1762,6 +1762,16 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent GenTree* const addr = parent->AsIndir()->Addr(); + // If we're directly loading through a local address, treat as an assigment. + // + if (addr->OperIs(GT_LCL_ADDR)) + { + JITDUMP("... local [indir] load\n"); + ++parentIndex; + ++numIndirs; + keepChecking = true; + break; + } // For loads from structs we may be tracking the underlying fields. // // We don't handle TYP_REF locals (yet), and allowing that requires separating out the object from @@ -1775,15 +1785,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // // We only track through the first indir. // - if (addr->OperIs(GT_LCL_ADDR)) - { - JITDUMP("... local [indir] load\n"); - ++parentIndex; - ++numIndirs; - keepChecking = true; - break; - } - else if (m_trackFields && (numIndirs == 0)) + else if (m_trackFields && (numIndirs == 0) && (lclDsc->TypeGet() != TYP_REF)) { JITDUMP("... local [struct] load\n"); ++parentIndex; diff --git a/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs b/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs index 2bdb6c1d407035..b08e85b25eef0e 100644 --- a/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs +++ b/src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs @@ -775,19 +775,19 @@ private static void ZeroAllocTest() { long before = GC.GetAllocatedBytesForCurrentThread(); Case1(); - EnsureZeroAllocated(before); + EnsureZeroAllocated(before, 1); Case2(); - EnsureZeroAllocated(before); + EnsureZeroAllocated(before, 2); Case3(null); - EnsureZeroAllocated(before); + EnsureZeroAllocated(before, 3); } [MethodImpl(MethodImplOptions.NoInlining)] - private static void EnsureZeroAllocated(long before) + private static void EnsureZeroAllocated(long before, int caseNumber) { long after = GC.GetAllocatedBytesForCurrentThread(); if (after - before != 0) - throw new InvalidOperationException($"Unexpected allocation: {after - before} bytes"); + throw new InvalidOperationException($"Unexpected allocation in Case {caseNumber}: {after - before} bytes"); } [MethodImpl(MethodImplOptions.NoInlining)] From 12f4c56c07d1275845deb6de086411c625d86941 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 31 Mar 2025 14:53:35 -0700 Subject: [PATCH 05/13] more null retyping (relops) --- src/coreclr/jit/objectalloc.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index b8aecb028be755..f1f5906a5de974 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -1909,6 +1909,29 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p case GT_GT: case GT_LE: case GT_GE: + { + // We may see sibling null refs. Retype them as appropriate. + // + GenTree* const lhs = parent->AsOp()->gtGetOp1(); + GenTree* const rhs = parent->AsOp()->gtGetOp2(); + + if (lhs == tree) + { + if (rhs->IsIntegralConst(0)) + { + rhs->ChangeType(newType); + } + } + else if (rhs == tree) + { + if (lhs->IsIntegralConst(0)) + { + lhs->ChangeType(newType); + } + } + break; + } + case GT_NULLCHECK: case GT_ARR_LENGTH: break; From adf2baffde2e2124702963634e826f91798930bf Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 1 Apr 2025 09:00:34 -0700 Subject: [PATCH 06/13] split out local var retyping from IR updates --- src/coreclr/jit/objectalloc.cpp | 178 ++++++++++++++++++++++---------- 1 file changed, 121 insertions(+), 57 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index f1f5906a5de974..aae0590277e072 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -2032,7 +2032,10 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p //------------------------------------------------------------------------ // RewriteUses: Find uses of the newobj temp for stack-allocated // objects and replace with address of the stack local. - +// +// Notes: +// Also retypes GC typed locals that now may or must refer to stack objects +// void ObjectAllocator::RewriteUses() { class RewriteUsesVisitor final : public GenTreeVisitor @@ -2063,69 +2066,40 @@ void ObjectAllocator::RewriteUses() } const unsigned int lclNum = tree->AsLclVarCommon()->GetLclNum(); - unsigned int newLclNum = BAD_VAR_NUM; LclVarDsc* lclVarDsc = m_compiler->lvaGetDesc(lclNum); - if (m_allocator->MayLclVarPointToStack(lclNum)) + // Revise IR for local that were retyped or are mapped to stack locals + // + if (!lclVarDsc->lvTracked) { - var_types newType = TYP_UNDEF; - if (m_allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum)) - { - assert(tree->OperIs(GT_LCL_VAR)); // Must be a use. - newType = TYP_I_IMPL; - tree = m_compiler->gtNewLclVarAddrNode(newLclNum); - *use = tree; - } - else - { - newType = m_allocator->DoesLclVarPointToStack(lclNum) ? TYP_I_IMPL : TYP_BYREF; - if (!tree->TypeIs(TYP_STRUCT)) - { - tree->ChangeType(newType); - } - } - - // For local structs, retype the GC fields. - // - if (lclVarDsc->lvType == TYP_STRUCT) - { - ClassLayout* const layout = lclVarDsc->GetLayout(); - ClassLayout* newLayout = nullptr; + return Compiler::fgWalkResult::WALK_CONTINUE; + } - if ((newType == TYP_I_IMPL) && !layout->IsBlockLayout()) - { - // New layout with no gc refs + padding - newLayout = m_compiler->typGetNonGCLayout(layout); - JITDUMP("Changing layout of struct V%02u to block\n", lclNum); - lclVarDsc->ChangeLayout(newLayout); - } - else if (!layout->IsCustomLayout()) // hacky... want to know if there are any TYP_GC - { - // New layout with all gc refs as byrefs + padding - // (todo, perhaps: see if old layout was already all byrefs) - newLayout = m_compiler->typGetByrefLayout(layout); - JITDUMP("Changing layout of struct V%02u to byref\n", lclNum); - lclVarDsc->ChangeLayout(newLayout); - } - } - // For locals, retype the local - // - else if (lclVarDsc->lvType != newType) - { - JITDUMP("Changing the type of V%02u from %s to %s\n", lclNum, varTypeName(lclVarDsc->lvType), - varTypeName(newType)); - lclVarDsc->lvType = newType; - } + unsigned int newLclNum = BAD_VAR_NUM; + var_types newType = lclVarDsc->TypeGet(); - m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType); + if (m_allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum)) + { + assert(tree->OperIs(GT_LCL_VAR)); // Must be a use. + newType = TYP_I_IMPL; + tree = m_compiler->gtNewLclVarAddrNode(newLclNum); + *use = tree; - if (newLclNum != BAD_VAR_NUM) - { - JITDUMP("Update V%02u to V%02u from use [%06u]\n", lclNum, newLclNum, m_compiler->dspTreeID(tree)); - DISPTREE(tree); - } + JITDUMP("Update V%02u to V%02u in use [%06u]\n", lclNum, newLclNum, m_compiler->dspTreeID(tree)); + DISPTREE(tree); + } + else if (newType == TYP_STRUCT) + { + ClassLayout* const layout = lclVarDsc->GetLayout(); + newType = layout->HasGCPtr() ? TYP_BYREF : TYP_I_IMPL; + } + else + { + tree->ChangeType(newType); } + m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType); + return Compiler::fgWalkResult::WALK_CONTINUE; } @@ -2229,6 +2203,96 @@ void ObjectAllocator::RewriteUses() } }; + // Determine which locals should be retyped, and retype them. + // Use lvTracked to remember which locals were retyped or will be replaced. + // + for (unsigned lclNum = 0; lclNum < comp->lvaCount; lclNum++) + { + LclVarDsc* const lclVarDsc = comp->lvaGetDesc(lclNum); + + if (!lclVarDsc->lvTracked) + { + JITDUMP("V%02u not tracked\n", lclNum); + continue; + } + + if (!MayLclVarPointToStack(lclNum)) + { + JITDUMP("V%02u not possibly stack pointing\n", lclNum); + lclVarDsc->lvTracked = 0; + continue; + } + + var_types newType = TYP_UNDEF; + if (m_HeapLocalToStackLocalMap.Contains(lclNum)) + { + // Appearances of lclNum will be replaced. We'll retype anyways. + // + newType = TYP_I_IMPL; + } + else + { + newType = DoesLclVarPointToStack(lclNum) ? TYP_I_IMPL : TYP_BYREF; + } + + // For local structs, retype the GC fields. + // + if (lclVarDsc->lvType == TYP_STRUCT) + { + assert(m_trackFields); + + ClassLayout* const layout = lclVarDsc->GetLayout(); + ClassLayout* newLayout = nullptr; + + if (!layout->HasGCPtr()) + { + assert(newType == TYP_I_IMPL); + JITDUMP("V%02u not GC\n", lclNum); + lclVarDsc->lvTracked = 0; + continue; + } + + if (newType == TYP_I_IMPL) + { + // New layout with no gc refs + padding + newLayout = comp->typGetNonGCLayout(layout); + JITDUMP("Changing layout of struct V%02u to block\n", lclNum); + lclVarDsc->ChangeLayout(newLayout); + } + else + { + // New layout with all gc refs as byrefs + padding + // (todo, perhaps: see if old layout was already all byrefs) + newLayout = comp->typGetByrefLayout(layout); + JITDUMP("Changing layout of struct V%02u to byref\n", lclNum); + lclVarDsc->ChangeLayout(newLayout); + } + } + else + { + // For non-struct locals, retype the local + // + if (!varTypeIsGC(lclVarDsc->TypeGet())) + { + JITDUMP("V%02u not GC\n", lclNum); + lclVarDsc->lvTracked = 0; + continue; + } + + if (lclVarDsc->lvType != newType) + { + // Params should only retype from ref->byref as they have unknown initial value + // + assert(!(lclVarDsc->lvIsParam && (newType == TYP_I_IMPL))); + JITDUMP("Changing the type of V%02u from %s to %s\n", lclNum, varTypeName(lclVarDsc->lvType), + varTypeName(newType)); + lclVarDsc->lvType = newType; + } + } + } + + // Update locals and types in the IR to match. + // for (BasicBlock* const block : comp->Blocks()) { for (Statement* const stmt : block->Statements()) @@ -2260,7 +2324,7 @@ void ObjectAllocator::RewriteUses() // that we must be able to clone the code and remove the potential for escape // // So, we verify for each case that we can clone; if not, mark we the pseudolocal -// as escaping. If any pseudlocal now escapes, we return true so that the main +// as escaping. If any pseudo local 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: From 63acf5bf77b0269b018c3064f274bad63ea255fb Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 5 Apr 2025 11:22:25 -0700 Subject: [PATCH 07/13] Stop trying to handle indirections through scalar locals --- src/coreclr/jit/objectalloc.cpp | 117 ++++++++++++++++++++++---------- src/coreclr/jit/objectalloc.h | 2 +- 2 files changed, 81 insertions(+), 38 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 7237138d14a512..8351576b1290bb 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -265,6 +265,7 @@ PhaseStatus ObjectAllocator::DoPhase() assert(enabled); ComputeStackObjectPointers(&m_bitVecTraits); RewriteUses(); + printf("***** Stack allocation in 0x%08x %s\n", comp->info.compMethodHash(), comp->info.compFullName); } // This phase always changes the IR. It may also modify the flow graph. @@ -500,7 +501,7 @@ void ObjectAllocator::PrepareAnalysis() if (m_nextLocalIndex > 0) { - JITDUMP("\nLocal var range [%02u...%02u]\n", 0, localCount); + JITDUMP("\nLocal var range [%02u...%02u]\n", 0, localCount - 1); if (m_maxPseudoLocals > 0) { JITDUMP("Enumerator var range [%02u...%02u]\n", localCount, localCount + m_maxPseudoLocals - 1); @@ -1586,7 +1587,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent bool canLclVarEscapeViaParentStack = true; bool isCopy = true; bool const isEnumeratorLocal = lclDsc->lvIsEnumerator; - int numIndirs = 0; + bool isLocalAddr = parentStack->Top()->OperIs(GT_LCL_ADDR); while (keepChecking) { @@ -1610,6 +1611,12 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent { case GT_STORE_LCL_VAR: { + if (isLocalAddr) + { + JITDUMP("... assigned &local\n"); + break; + } + const unsigned int dstLclNum = parent->AsLclVar()->GetLclNum(); // If we're not tracking stores to this local, the value @@ -1664,7 +1671,6 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent case GT_QMARK: case GT_ADD: case GT_FIELD_ADDR: - case GT_LCL_ADDR: // Check whether the local escapes higher up ++parentIndex; keepChecking = true; @@ -1713,26 +1719,21 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent break; } - JITDUMP("... store value\n"); - // Is this a store to a field of a local struct...? // if (parent->OperIs(GT_STOREIND)) { - if (addr->OperIs(GT_LCL_ADDR)) + // Are we storing the address of a local? + // + if (isLocalAddr) { - unsigned const dstLclNum = addr->AsLclVarCommon()->GetLclNum(); - LclVarDsc* const dstDsc = comp->lvaGetDesc(dstLclNum); - - if (IsTrackedLocal(dstLclNum)) - { - JITDUMP("... local [indir] store\n"); - // Add an edge to the connection graph. - AddConnGraphEdge(dstLclNum, lclNum); - canLclVarEscapeViaParentStack = false; - } + JITDUMP("... &local store value\n"); + break; } - else if (addr->OperIs(GT_FIELD_ADDR)) + + // Are we storing to a local field? + // + if (addr->OperIs(GT_FIELD_ADDR)) { // Simple check for which local. // @@ -1745,13 +1746,16 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent if (IsTrackedLocal(dstLclNum)) { - JITDUMP("... local [struct] store\n"); + JITDUMP("... local.field store\n"); // Add an edge to the connection graph. AddConnGraphEdge(dstLclNum, lclNum); canLclVarEscapeViaParentStack = false; } } } + + // Else we're storing the value somewhere unknown. + // Assume the worst. } break; } @@ -1762,47 +1766,47 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // if (!IsTrackedType(parent->TypeGet())) { + JITDUMP("... indir result not gc\n"); canLclVarEscapeViaParentStack = false; break; } - GenTree* const addr = parent->AsIndir()->Addr(); - - // If we're directly loading through a local address, treat as an assigment. + // Is the indir address based on a local address? // - if (addr->OperIs(GT_LCL_ADDR)) + if (!isLocalAddr) { - JITDUMP("... local [indir] load\n"); - ++parentIndex; - ++numIndirs; - keepChecking = true; + JITDUMP("... indir addr not derived from &local\n"); + canLclVarEscapeViaParentStack = false; break; } + + GenTree* const addr = parent->AsIndir()->Addr(); + // For loads from structs we may be tracking the underlying fields. // // We don't handle TYP_REF locals (yet), and allowing that requires separating out the object from // its fields in our tracking. // // We treat TYP_BYREF like TYP_STRUCT, though possibly this needs more scrutiny, as byrefs may alias. - // Ditto for TYP_I_IMPL. // // We can assume that the local being read is lclNum, since we have walked up to this node from a leaf // local. // // We only track through the first indir. // - else if (m_trackFields && (numIndirs == 0) && (lclDsc->TypeGet() != TYP_REF)) + // If we're directly loading through a local address, treat as an assigment. + // + if (m_trackFields && isLocalAddr && addr->OperIs(GT_FIELD_ADDR) && (lclDsc->TypeGet() != TYP_REF)) { - JITDUMP("... local [struct] load\n"); + JITDUMP("... load local.field\n"); ++parentIndex; - ++numIndirs; keepChecking = true; + isLocalAddr = false; break; } - // Address doesn't refer to any location we track + // Unknown address tree involving a local addr, assume the worst. // - canLclVarEscapeViaParentStack = false; break; } @@ -1870,6 +1874,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // tree - Possibly-stack-pointing tree // parentStack - Parent stack of the possibly-stack-pointing tree // newType - New type of the possibly-stack-pointing tree +// isStruct - true if inspiring local is a retyped struct // // Notes: // If newType is TYP_I_IMPL, the tree is definitely pointing to the stack (or is null); @@ -1877,12 +1882,14 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // In addition to updating types this method may set GTF_IND_TGT_NOT_HEAP on ancestor // indirections to help codegen with write barrier selection. // -void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* parentStack, var_types newType) +void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, + ArrayStack* parentStack, + var_types newType, + bool isStruct) { assert(newType == TYP_BYREF || newType == TYP_I_IMPL); assert(parentStack != nullptr); - int parentIndex = 1; - + int parentIndex = 1; bool keepChecking = true; while (keepChecking && (parentStack->Height() > parentIndex)) @@ -1952,7 +1959,6 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p case GT_QMARK: case GT_ADD: case GT_FIELD_ADDR: - case GT_INDEX_ADDR: case GT_LCL_ADDR: case GT_BOX: if (parent->TypeGet() != newType) @@ -1963,6 +1969,16 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p keepChecking = true; break; + case GT_INDEX_ADDR: + // We are not retyping array "fields" yet + // so we can stop updating here. + // + if (parent->TypeGet() != newType) + { + parent->ChangeType(newType); + } + break; + case GT_SUB: { // Parent type can be TYP_I_IMPL, TYP_BYREF. @@ -2019,6 +2035,7 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p case GT_STOREIND: case GT_STORE_BLK: case GT_BLK: + { if (tree == parent->AsIndir()->Addr()) { // The new target could be *not* on the heap. @@ -2032,9 +2049,33 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* p parent->gtFlags |= GTF_IND_TGT_NOT_HEAP; } } + else + { + assert(tree == parent->AsIndir()->Data()); + + // If we are storing to a GC struct field, we may need to retype the store + // + if (parent->OperIs(GT_STOREIND) && isStruct && (varTypeIsGC(parent->TypeGet()))) + { + parent->ChangeType(newType); + } + } break; + } case GT_IND: + { + // If we are loading from a GC struct field, we may need to retype the load + // + if (isStruct && (varTypeIsGC(parent->TypeGet()))) + { + parent->ChangeType(newType); + ++parentIndex; + keepChecking = true; + } + break; + } + case GT_CALL: break; @@ -2101,6 +2142,7 @@ void ObjectAllocator::RewriteUses() unsigned int newLclNum = BAD_VAR_NUM; var_types newType = lclVarDsc->TypeGet(); + bool isStruct = false; if (m_allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum)) { @@ -2116,13 +2158,14 @@ void ObjectAllocator::RewriteUses() { ClassLayout* const layout = lclVarDsc->GetLayout(); newType = layout->HasGCPtr() ? TYP_BYREF : TYP_I_IMPL; + isStruct = true; } else { tree->ChangeType(newType); } - m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType); + m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType, isStruct); return Compiler::fgWalkResult::WALK_CONTINUE; } diff --git a/src/coreclr/jit/objectalloc.h b/src/coreclr/jit/objectalloc.h index e06fd943b95146..d4940144a295dc 100644 --- a/src/coreclr/jit/objectalloc.h +++ b/src/coreclr/jit/objectalloc.h @@ -200,7 +200,7 @@ class ObjectAllocator final : public Phase Statement* stmt); struct BuildConnGraphVisitorCallbackData; bool CanLclVarEscapeViaParentStack(ArrayStack* parentStack, unsigned int lclNum, BasicBlock* block); - void UpdateAncestorTypes(GenTree* tree, ArrayStack* parentStack, var_types newType); + void UpdateAncestorTypes(GenTree* tree, ArrayStack* parentStack, var_types newType, bool isStruct); ObjectAllocationType AllocationKind(GenTree* tree); // Conditionally escaping allocation support From 1fcf375f75b3726cc1baae53c36f9ae3a3856b82 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 6 Apr 2025 16:17:45 -0700 Subject: [PATCH 08/13] remove debugging printf --- src/coreclr/jit/objectalloc.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 8351576b1290bb..44d6cb1fe93962 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -265,7 +265,6 @@ PhaseStatus ObjectAllocator::DoPhase() assert(enabled); ComputeStackObjectPointers(&m_bitVecTraits); RewriteUses(); - printf("***** Stack allocation in 0x%08x %s\n", comp->info.compMethodHash(), comp->info.compFullName); } // This phase always changes the IR. It may also modify the flow graph. From 6305cda57c655d4deda28515c5c7780601c051af Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 7 Apr 2025 09:09:10 -0700 Subject: [PATCH 09/13] stop retyping indirs past the first --- src/coreclr/jit/objectalloc.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 44d6cb1fe93962..c1ec06815e5e66 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -2070,6 +2070,8 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, { parent->ChangeType(newType); ++parentIndex; + // We are no longer referring to the original local + isStruct = false; keepChecking = true; } break; From 6a509d2c2a7f4338d679d6cfb7fdc9ef4e62c410 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 7 Apr 2025 09:30:13 -0700 Subject: [PATCH 10/13] format --- src/coreclr/jit/objectalloc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index c1ec06815e5e66..5ff706251ab81f 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -2071,7 +2071,7 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, parent->ChangeType(newType); ++parentIndex; // We are no longer referring to the original local - isStruct = false; + isStruct = false; keepChecking = true; } break; From a6cd8d9aa8fba266052bcb5f4ac1e8c021139506 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 8 Apr 2025 13:14:17 -0700 Subject: [PATCH 11/13] handle indir stores to &local better --- src/coreclr/jit/objectalloc.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index 5ff706251ab81f..a5b3eaba8d6f38 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -1713,8 +1713,20 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent GenTree* const addr = parent->AsIndir()->Addr(); if (tree == addr) { + // Address tree doesn't escape. + // JITDUMP("... store address\n"); canLclVarEscapeViaParentStack = false; + + if (isLocalAddr && !addr->OperIs(GT_FIELD_ADDR)) + { + // We are indirectly storing to a tracked local. + // For now, assume we don't know what value is stored. + // + JITDUMP("... store &local\n"); + AddConnGraphEdge(lclNum, m_unknownSourceLocalNum); + } + break; } @@ -2047,6 +2059,13 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, // It's either null or points to inside a stack-allocated object. parent->gtFlags |= GTF_IND_TGT_NOT_HEAP; } + + // If we have an indirect store to a retyped local, retype the store. + // + if (tree->OperIs(GT_LCL_ADDR) && (parent->TypeGet() != newType)) + { + parent->ChangeType(newType); + } } else { From be237f6225fa42f69c6c209f6e519754401da9a1 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 10 Apr 2025 17:40:38 -0700 Subject: [PATCH 12/13] try and simplify handling of address-taken scalars --- src/coreclr/jit/objectalloc.cpp | 109 +++++++++++++------------------- src/coreclr/jit/objectalloc.h | 2 +- 2 files changed, 44 insertions(+), 67 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index a5b3eaba8d6f38..b984cbeb623eda 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -596,8 +596,9 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) { - GenTree* const tree = *use; - unsigned const lclNum = tree->AsLclVarCommon()->GetLclNum(); + GenTree* const tree = *use; + unsigned const lclNum = tree->AsLclVarCommon()->GetLclNum(); + LclVarDsc* const lclDsc = m_compiler->lvaGetDesc(lclNum); // If this local already escapes, no need to look further. // @@ -613,7 +614,16 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() lclEscapes = false; m_allocator->CheckForGuardedAllocationOrCopy(m_block, m_stmt, use, lclNum); } - else if (tree->OperIs(GT_LCL_VAR, GT_LCL_ADDR) && m_allocator->IsTrackedLocal(lclNum)) + else if (tree->OperIs(GT_LCL_VAR) && m_allocator->IsTrackedLocal(lclNum)) + { + assert(tree == m_ancestors.Top()); + if (!m_allocator->CanLclVarEscapeViaParentStack(&m_ancestors, lclNum, m_block)) + { + lclEscapes = false; + } + } + else if (tree->OperIs(GT_LCL_ADDR) && (lclDsc->TypeGet() == TYP_STRUCT) && + m_allocator->IsTrackedLocal(lclNum)) { assert(tree == m_ancestors.Top()); if (!m_allocator->CanLclVarEscapeViaParentStack(&m_ancestors, lclNum, m_block)) @@ -1586,7 +1596,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent bool canLclVarEscapeViaParentStack = true; bool isCopy = true; bool const isEnumeratorLocal = lclDsc->lvIsEnumerator; - bool isLocalAddr = parentStack->Top()->OperIs(GT_LCL_ADDR); + bool isAddress = parentStack->Top()->OperIs(GT_LCL_ADDR); while (keepChecking) { @@ -1610,16 +1620,17 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent { case GT_STORE_LCL_VAR: { - if (isLocalAddr) + // If the store value is a local address, anything assigned to that local escapes + // + if (isAddress) { - JITDUMP("... assigned &local\n"); break; } const unsigned int dstLclNum = parent->AsLclVar()->GetLclNum(); - // If we're not tracking stores to this local, the value - // does not escape. + // If we're not tracking stores to the dest local, the value does not escape. + // if (!IsTrackedLocal(dstLclNum)) { canLclVarEscapeViaParentStack = false; @@ -1713,20 +1724,15 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent GenTree* const addr = parent->AsIndir()->Addr(); if (tree == addr) { - // Address tree doesn't escape. - // JITDUMP("... store address\n"); canLclVarEscapeViaParentStack = false; + break; + } - if (isLocalAddr && !addr->OperIs(GT_FIELD_ADDR)) - { - // We are indirectly storing to a tracked local. - // For now, assume we don't know what value is stored. - // - JITDUMP("... store &local\n"); - AddConnGraphEdge(lclNum, m_unknownSourceLocalNum); - } - + // If the value being stored is a local address, anything assigned to that local escapes + // + if (isAddress) + { break; } @@ -1734,14 +1740,6 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // if (parent->OperIs(GT_STOREIND)) { - // Are we storing the address of a local? - // - if (isLocalAddr) - { - JITDUMP("... &local store value\n"); - break; - } - // Are we storing to a local field? // if (addr->OperIs(GT_FIELD_ADDR)) @@ -1777,47 +1775,32 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // if (!IsTrackedType(parent->TypeGet())) { - JITDUMP("... indir result not gc\n"); - canLclVarEscapeViaParentStack = false; - break; - } - - // Is the indir address based on a local address? - // - if (!isLocalAddr) - { - JITDUMP("... indir addr not derived from &local\n"); canLclVarEscapeViaParentStack = false; break; } GenTree* const addr = parent->AsIndir()->Addr(); - // For loads from structs we may be tracking the underlying fields. - // - // We don't handle TYP_REF locals (yet), and allowing that requires separating out the object from - // its fields in our tracking. - // - // We treat TYP_BYREF like TYP_STRUCT, though possibly this needs more scrutiny, as byrefs may alias. + // For loads from local structs we may be tracking the underlying fields. + // // We can assume that the local being read is lclNum, since we have walked up to this node from a leaf // local. // // We only track through the first indir. // - // If we're directly loading through a local address, treat as an assigment. - // - if (m_trackFields && isLocalAddr && addr->OperIs(GT_FIELD_ADDR) && (lclDsc->TypeGet() != TYP_REF)) + if (m_trackFields && isAddress && addr->OperIs(GT_FIELD_ADDR) && (lclDsc->TypeGet() != TYP_REF)) { JITDUMP("... load local.field\n"); ++parentIndex; + isAddress = false; keepChecking = true; - isLocalAddr = false; break; } - // Unknown address tree involving a local addr, assume the worst. + // Address doesn't refer to any location we track // + canLclVarEscapeViaParentStack = false; break; } @@ -1885,7 +1868,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent // tree - Possibly-stack-pointing tree // parentStack - Parent stack of the possibly-stack-pointing tree // newType - New type of the possibly-stack-pointing tree -// isStruct - true if inspiring local is a retyped struct +// retypeFields - Inspiring local is a retyped local struct; retype fields. // // Notes: // If newType is TYP_I_IMPL, the tree is definitely pointing to the stack (or is null); @@ -1896,7 +1879,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, ArrayStack* parentStack, var_types newType, - bool isStruct) + bool retypeFields) { assert(newType == TYP_BYREF || newType == TYP_I_IMPL); assert(parentStack != nullptr); @@ -2059,21 +2042,16 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, // It's either null or points to inside a stack-allocated object. parent->gtFlags |= GTF_IND_TGT_NOT_HEAP; } - - // If we have an indirect store to a retyped local, retype the store. - // - if (tree->OperIs(GT_LCL_ADDR) && (parent->TypeGet() != newType)) - { - parent->ChangeType(newType); - } } else { assert(tree == parent->AsIndir()->Data()); + GenTree* const addr = parent->AsIndir()->Addr(); // If we are storing to a GC struct field, we may need to retype the store // - if (parent->OperIs(GT_STOREIND) && isStruct && (varTypeIsGC(parent->TypeGet()))) + if (retypeFields && parent->OperIs(GT_STOREIND) && (addr->OperIs(GT_FIELD_ADDR)) && + (varTypeIsGC(parent->TypeGet()))) { parent->ChangeType(newType); } @@ -2085,13 +2063,12 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, { // If we are loading from a GC struct field, we may need to retype the load // - if (isStruct && (varTypeIsGC(parent->TypeGet()))) + if (retypeFields && (tree->OperIs(GT_FIELD_ADDR)) && (varTypeIsGC(parent->TypeGet()))) { parent->ChangeType(newType); ++parentIndex; - // We are no longer referring to the original local - isStruct = false; keepChecking = true; + retypeFields = false; } break; } @@ -2150,8 +2127,9 @@ void ObjectAllocator::RewriteUses() return Compiler::fgWalkResult::WALK_CONTINUE; } - const unsigned int lclNum = tree->AsLclVarCommon()->GetLclNum(); - LclVarDsc* lclVarDsc = m_compiler->lvaGetDesc(lclNum); + const unsigned int lclNum = tree->AsLclVarCommon()->GetLclNum(); + LclVarDsc* lclVarDsc = m_compiler->lvaGetDesc(lclNum); + bool retypeFields = false; // Revise IR for local that were retyped or are mapped to stack locals // @@ -2162,7 +2140,6 @@ void ObjectAllocator::RewriteUses() unsigned int newLclNum = BAD_VAR_NUM; var_types newType = lclVarDsc->TypeGet(); - bool isStruct = false; if (m_allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum)) { @@ -2178,14 +2155,14 @@ void ObjectAllocator::RewriteUses() { ClassLayout* const layout = lclVarDsc->GetLayout(); newType = layout->HasGCPtr() ? TYP_BYREF : TYP_I_IMPL; - isStruct = true; + retypeFields = true; } else { tree->ChangeType(newType); } - m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType, isStruct); + m_allocator->UpdateAncestorTypes(tree, &m_ancestors, newType, retypeFields); return Compiler::fgWalkResult::WALK_CONTINUE; } diff --git a/src/coreclr/jit/objectalloc.h b/src/coreclr/jit/objectalloc.h index d4940144a295dc..624368ca37ed65 100644 --- a/src/coreclr/jit/objectalloc.h +++ b/src/coreclr/jit/objectalloc.h @@ -200,7 +200,7 @@ class ObjectAllocator final : public Phase Statement* stmt); struct BuildConnGraphVisitorCallbackData; bool CanLclVarEscapeViaParentStack(ArrayStack* parentStack, unsigned int lclNum, BasicBlock* block); - void UpdateAncestorTypes(GenTree* tree, ArrayStack* parentStack, var_types newType, bool isStruct); + void UpdateAncestorTypes(GenTree* tree, ArrayStack* parentStack, var_types newType, bool retypeFields); ObjectAllocationType AllocationKind(GenTree* tree); // Conditionally escaping allocation support From 105ea69eb9e28acf2434fe8a61e41e3d2f9409be Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 10 Apr 2025 17:56:34 -0700 Subject: [PATCH 13/13] cleanup; format --- src/coreclr/jit/objectalloc.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index b984cbeb623eda..e1f538aea8d18d 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -1782,14 +1782,13 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack* parent GenTree* const addr = parent->AsIndir()->Addr(); // For loads from local structs we may be tracking the underlying fields. - // // We can assume that the local being read is lclNum, since we have walked up to this node from a leaf // local. // // We only track through the first indir. // - if (m_trackFields && isAddress && addr->OperIs(GT_FIELD_ADDR) && (lclDsc->TypeGet() != TYP_REF)) + if (m_trackFields && isAddress && addr->OperIs(GT_FIELD_ADDR)) { JITDUMP("... load local.field\n"); ++parentIndex; @@ -1953,7 +1952,6 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, case GT_QMARK: case GT_ADD: case GT_FIELD_ADDR: - case GT_LCL_ADDR: case GT_BOX: if (parent->TypeGet() != newType) {