-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Don't set CORINFO_FLG_CUSTOMLAYOUT for auto-layout structs #71673
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsApply @SingleAccretion's advise in #71667 (comment) to see if it works (works locally, also I tests various kinds of structs to see when "ExplicitSize" is reported and when not) - if it passes the tests I will run jit-diff tools locally to estimate impact.
|
Fun fact: crossgen is not affected - it checks differently runtime/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Lines 2097 to 2099 in 2170ea9
|
src/coreclr/vm/jitinterface.cpp
Outdated
pMT == g_TypedReferenceMT || | ||
VMClsHnd.IsNativeValueType()) | ||
{ | ||
// All kinds of structs with Size=.., Pack=... will be marked as "Explicit Size" | ||
// and end up as "custom layout". We consider "Auto-layout" as non-custom one |
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.
We consider "Auto-layout" as non-custom one
Is this an okay assumption?
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.
it's always stable and Size=...
will be no-op and user unlikely will use it in inappropriate ways like storing some custom data in that padding of Span
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.
Actually, it might be not a correct assumption 🤔 let me check what might happen with auto-layout
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.
This condition is very fragile. The meaning of CORINFO_FLG_CUSTOMLAYOUT
is very poorly defined. I do not think you want to be touching it to get the regression fixed quickly.
It sounds like that some flags on Span<T>
MethodTable
changed and that introduced the regression. What are the flags on MethodTable
that changed?
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.
@jkotas in JIT we have a logic:
if (StructHasCustomLayout(typeFlags) && ((typeFlags & CORINFO_FLG_CONTAINS_GC_PTR) == 0))
{
structPromotionInfo.customLayout = true;
}
our assumption that with the previous impl it was not reported as CORINFO_FLG_CONTAINS_GC_PTR
with e.g. ByReference<byte>
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.
So for Span<byte>
:
pMT->ContainsPointers(); // was: 0, now: 0
pClass->IsManagedSequential(); // was: 1, now: 0
pClass->HasExplicitSize(); // was: 0, now: 0
pClass->IsAutoLayoutOrHasAutoLayoutField(); // was: 0, now: 0
pMT->dwFlags // was: 266256, now: 266256
Seems like the only thing changed is IsManagedSequential
to 0
that with current logic leads to CORINFO_FLG_CUSTOMLAYOUT
being sent to JIT.
From my understanding, the primary usage for this flag is to tell jit that e.g. holes in current struct are likely "valueable" so it better not promote it.
As I noted earlier, the check is different in crossgen (looks better for jit):
runtime/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
Lines 2097 to 2099 in 2170ea9
// The CLR has more complicated rules around CUSTOMLAYOUT, but this will do. | |
if (metadataType.IsExplicitLayout || (metadataType.IsSequentialLayout && metadataType.GetClassLayout().Size != 0) || metadataType.IsWellKnownType(WellKnownType.TypedReference)) | |
result |= CorInfoFlag.CORINFO_FLG_CUSTOMLAYOUT; |
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.
Seems like the only thing changed is IsManagedSequential to 0 that with current logic leads to
So the simple fix should be to change this back. I think that the following change in TypeHasGCPointers
is going to do that:
< if (CorTypeInfo::IsPrimitiveType(corElemType) || corElemType == ELEMENT_TYPE_PTR || corElemType == ELEMENT_TYPE_FNPTR)
> if (CorTypeInfo::IsPrimitiveType(corElemType) || corElemType == ELEMENT_TYPE_PTR || corElemType == ELEMENT_TYPE_FNPTR || corElemType == ELEMENT_TYPE_BYREF)
The custom layout flag is a mess and it would be nice to revisit it, but it should be done in a separate PR.
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.
We no longer classify System.Span`1
as managed sequential because TypeHasGCPointers(ELEMENT_TYPE_BYREF, ...)
returns true, so we set fDisqualifyFromManagedSequential
in DetermineBlittabilityAndManagedSequential
. Before the change TypeHasGCPointers(ELEMENT_TYPE_VALUETYPE, <type handle for System.ByReference`1>)
was returning false.
EDIT: Ah, looks like Jan beat me to it :-)
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.
Notice that the logic in
runtime/src/coreclr/vm/classlayoutinfo.cpp
Lines 304 to 312 in 27195f6
if (CorTypeInfo::IsPrimitiveType(corElemType) || corElemType == ELEMENT_TYPE_PTR || corElemType == ELEMENT_TYPE_FNPTR) | |
{ | |
return FALSE; | |
} | |
if (corElemType == ELEMENT_TYPE_VALUETYPE) | |
{ | |
_ASSERTE(!pNestedType.IsNull()); | |
return pNestedType.GetMethodTable()->ContainsPointers() != FALSE; | |
} |
The change that I have proposed above is going to fix this inconsistency.
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.
The custom layout flag is a mess and it would be nice to revisit it, but it should be done in a separate PR.
I have opened #71711 to track this.
/azp run runtime-coreclr outerloop, runtime-libraries-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr jitstressregs |
Azure Pipelines successfully started running 4 pipeline(s). |
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.
Assuming that it passes the CI and fixes the codegen regression
Thanks for the feedback, it does fix the regression! |
Could we get |
The diffs look massive for this change (Main vs this PR):
Checked a few random regressions and they look inlining-related |
Failure is #71684 |
jit-diff for before and after #71498
So looks like the regression is properly fixed |
Yeah, looks like it. There is still some difference with a total net of |
another regression: #71673 |
@AndyAyersMS I think that is a self-referencing link :-) |
So it is... sadly I can't find the actual issue I meant to link. |
Close #71667
Apply @SingleAccretion's suggestion in #71667 (comment) to see if it works (works locally, also I tested various kinds of structs to see when "ExplicitSize" is reported and when not) - if it passes the tests I will run jit-diff tools locally to estimate impact.