Skip to content

JIT: enhance escape analysis to understand local struct fields #113141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

JIT: enhance escape analysis to understand local struct fields #113141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5dbbc81
JIT: enhance escape analysis to understand `Span<T>` capture
AndyAyersMS Feb 13, 2025
5fcfe24
Generalize to handling all GC structs.
AndyAyersMS Mar 5, 2025
7b0172a
fix GT_SUB type propagation
AndyAyersMS Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,6 +1048,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)
{
Expand Down Expand Up @@ -10960,8 +10967,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);
Comment on lines +10970 to +10973
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like something that only stack allocation is ever going to use, so it would make more sense to me to keep it in objectalloc.h/cpp.


var_types TypeHandleToVarType(CORINFO_CLASS_HANDLE handle, ClassLayout** pLayout = nullptr);
var_types TypeHandleToVarType(CorInfoType jitType, CORINFO_CLASS_HANDLE handle, ClassLayout** pLayout = nullptr);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/jitconfigvalues.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,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)

Expand Down
137 changes: 120 additions & 17 deletions src/coreclr/jit/layout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<char>(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<char>(len);
sprintf_s(newShortName, len, "%s%s", prefix, layoutShortName);
}

SetName(newName, newShortName);
}
else
{
SetName(layoutName, layoutShortName);
}
Comment on lines +463 to +487
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can use printfAlloc.

}
#endif // DEBUG

//------------------------------------------------------------------------
// Create: Create a ClassLayout from an EE side class handle.
//
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -706,8 +768,8 @@ bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* l
return true;
}

assert(clsHnd1 != NO_CLASS_HANDLE);
assert(clsHnd2 != NO_CLASS_HANDLE);
// assert(clsHnd1 != NO_CLASS_HANDLE);
// assert(clsHnd2 != NO_CLASS_HANDLE);
assert(layout1->HasGCPtr() && layout2->HasGCPtr());

if (layout1->GetGCPtrCount() != layout2->GetGCPtrCount())
Expand All @@ -722,6 +784,13 @@ bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* l
{
if (layout1->GetGCPtrType(i) != layout2->GetGCPtrType(i))
{
// Allow a source GC_REF to match a custom GC_BYREF
//
if ((layout2->GetGCPtrType(i) == TYP_REF) && (layout1->GetGCPtrType(i) == TYP_BYREF) &&
layout1->IsCustomLayout())
{
continue;
}
Comment on lines +787 to +793
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the definition of layout compatibility seems quite questionable to me. Why is it necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

GCStruct a;
a.obj = ..;   // doesn't escape

GCStruct b;
b.obj = ..;   // escapes

// Assignment is legal, but layouts are not compatible w/o the above
// a.obj must be BYREF as it may refer to stack or heap

a = b

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tempted to retain the class handle in new layout, but didn't.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note allowing b = a would be incompatible with the results of escape analysis, which is why this is one-sided (source can be byref when dest is ref, but not vice-versa).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you saying that for the

object aObj = ...; // doesn't escape

object bObj = ...; // escapes

aObj = bObj;

case, today we will produce a STORE_LCL_VAR that stores a TYP_REF into a TYP_BYREF?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just suggest to rename this to ClassLayout::AssignmentCompatible(ClassLayout* dest, ClassLayout* src) and allow the byref <- ref assignments generally. And then switch Unsafe.BitCast to a version that keeps the stricter behavior.
Should probably be a separate PR...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Maybe BitCast doesn't even need any changes, I was under the assumption that it threw an exception on incompatible layouts, but it does not seem like it is the case)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea here is to leverage the fact that these new layouts are based on existing layouts, and remember the "inspiring" layout.

Then if two layouts have the same inspiring layout, they are compatible, provided we're not assigning a gc layout to a non-gc layout (which should indicate an error in escape analysis).

Also tempted to make this an instance method so it's easier to remember which layout is which, eg
destLayout->CanAssignFrom(sourceLayout).

Copy link
Member

@jakobbotsch jakobbotsch Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be odd to me if a layout inspired by another layout would compare differently than another, equal layout that was not inspired. That would break transitivity properties you would expect to hold.

I am fine with introducing a notion of assignment compatibility given that we allow the BYREF <- REF stores today (which I have to admit still worries me somewhat...). But I think we should keep the equivalence relation of "compatibility" around as well. I was looking at uses of AreCompatible at some point and it seemed like there were definitely places that want the equivalence relation rather than some form of assignment compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me split out the existing uses in a separate PR.

I need to look at what escape analysis will do for struct retyping that preserves GCness to see if we ever end up with a case like

GCStruct    a;   // does not escape
GCStructAlt b;   // escapes,

b = Unsafe.BitCast(a);

and make sure we know to retype b.

return false;
}
}
Expand Down Expand Up @@ -791,7 +860,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;
}
}
Expand Down Expand Up @@ -896,14 +965,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);

Expand All @@ -916,18 +984,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);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code feels very object stack allocation specific. Put it in objectalloc.cpp?


//------------------------------------------------------------------------
// 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.
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/jit/layout.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading
Loading