-
Notifications
You must be signed in to change notification settings - Fork 5.1k
JIT: enhance escape analysis to understand local struct fields #113977
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.
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
Changes from 1 commit
812df7b
b6bf10b
70e2d14
e0432bd
12f4c56
adf2baf
d3727d3
63acf5b
1fcf375
6305cda
6a509d2
a6cd8d9
fc0f13a
be237f6
105ea69
9fa1a40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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); | ||||||
} | ||||||
Comment on lines
+468
to
+480
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, will do that in the follow-up. |
||||||
|
||||||
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) | ||||||
Comment on lines
+711
to
+712
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Undo this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will address this in the follow-up. |
||||||
// | ||||||
// 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; | ||||||
} | ||||||
} | ||||||
Comment on lines
+818
to
+828
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't the logic below subsume this check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to make this a superset of what I can either just give up on that idea or else give There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the assert? I would probably vote to just take the small amount of duplication. Seems simpler to reason about the operation that way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. runtime/src/coreclr/jit/layout.cpp Lines 712 to 713 in f79c098
Without the guarding we can get here with a custom (byref) source and a class-based (ref) dest. The "early" out would be just before this, since in this situation an exact gc slot compare will fail; we need the more relaxed one that allows mixed types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that assert is just outdated, it should have been removed as part of #112064 since we can now have layouts without class handles but with GC pointers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case I can just remove the guarding and call it unconditionally. |
||||||
|
||||||
// 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); | ||||||
} | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as previously.. This feels awfully specific to escape analysis to add in the generic builder type. |
||||||
|
||||||
//------------------------------------------------------------------------ | ||||||
// 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. | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace
GrowBlockLayout
with this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure ... neither is widely used, both likely get inlined and evaporate in release.