Skip to content
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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+698 −75
Open

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 Author

@AndyAyersMS AndyAyersMS Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we will retype aObj appearances to be byrefs, as it will be "possibly stack pointing". And aObj itself in the local table.

This has been there since forever.

Copy link
Member

@jakobbotsch jakobbotsch Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, it makes it somewhat more palatable. But it seems like we need to separate the notions of "user facing" layout compatibility, as used by e.g. Unsafe.BitCast, and "implementation-specific assignment compatibility" which is non-symmetric and where we rely on internal coreclr implementation details. As you probably know there could be a potential future where byrefs pointing at the method table would be illegal... I guess in that case we would need a third kind of GC pointer. Or we would have to switch object stack allocation to only kick in when it can reason about and change all accesses of the object.

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'm open to suggestions on how to express this better. Perhaps the presence of a custom layout for the dest might be enough?

We could get here with custom layouts on both sides:

GCStruct a;
a.obj = ..;   // doesn't escape, may point at heap [byref]

GCStruct b;
b.obj = ..;   // doesn't escape, never points at heap [nongc]

a = b

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)

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