Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,7 @@ enum CorInfoFlag
CORINFO_FLG_OVERLAPPING_FIELDS = 0x00100000, // struct or class has fields that overlap (aka union)
CORINFO_FLG_INTERFACE = 0x00200000, // it is an interface
CORINFO_FLG_DONT_DIG_FIELDS = 0x00400000, // don't ask field info, AOT can't rely on it (used for types outside of AOT compilation version bubble)
CORINFO_FLG_CUSTOMLAYOUT = 0x00800000, // does this struct have custom layout?
CORINFO_FLG_OPAQUE_BLOB = 0x00800000, // should this struct be treated as opaque blob of bytes?
CORINFO_FLG_CONTAINS_GC_PTR = 0x01000000, // does the class contain a gc ptr ?
CORINFO_FLG_DELEGATE = 0x02000000, // is this a subclass of delegate or multicast delegate ?
// CORINFO_FLG_UNUSED = 0x04000000,
Expand Down
11 changes: 3 additions & 8 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ class LclVarDsc
unsigned char lvIsStructField : 1; // Is this local var a field of a promoted struct local?
unsigned char lvOverlappingFields : 1; // True when we have a struct with possibly overlapping fields
unsigned char lvContainsHoles : 1; // True when we have a promoted struct that contains holes
unsigned char lvCustomLayout : 1; // True when this struct has "CustomLayout"
unsigned char lvOpaqueBlob : 1;

unsigned char lvIsMultiRegArg : 1; // true if this is a multireg LclVar struct used in an argument context
unsigned char lvIsMultiRegRet : 1; // true if this is a multireg LclVar struct assigned from a multireg call
Expand Down Expand Up @@ -4103,18 +4103,13 @@ class Compiler
CORINFO_CLASS_HANDLE typeHnd;
bool canPromote;
bool containsHoles;
bool customLayout;
bool opaqueBlob;
bool fieldsSorted;
unsigned char fieldCnt;
lvaStructFieldInfo fields[MAX_NumOfFieldsInPromotableStruct];

lvaStructPromotionInfo(CORINFO_CLASS_HANDLE typeHnd = nullptr)
: typeHnd(typeHnd)
, canPromote(false)
, containsHoles(false)
, customLayout(false)
, fieldsSorted(false)
, fieldCnt(0)
: typeHnd(typeHnd), canPromote(false), containsHoles(false), opaqueBlob(false), fieldsSorted(false), fieldCnt(0)
{
}
};
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4480,9 +4480,10 @@ inline static bool StructHasOverlappingFields(DWORD attribs)
return ((attribs & CORINFO_FLG_OVERLAPPING_FIELDS) != 0);
}

inline static bool StructHasCustomLayout(DWORD attribs)
inline static bool TreatStructAsOpaqueBlobOfBytes(DWORD attribs)

{
return ((attribs & CORINFO_FLG_CUSTOMLAYOUT) != 0);
return ((attribs & CORINFO_FLG_OPAQUE_BLOB) != 0);
}

inline static bool StructHasDontDigFieldsFlagSet(DWORD attribs)
Expand Down
36 changes: 5 additions & 31 deletions src/coreclr/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1716,9 +1716,9 @@ bool Compiler::StructPromotionHelper::CanPromoteStructType(CORINFO_CLASS_HANDLE
return false;
}

// Don't struct promote if we have an CUSTOMLAYOUT flag on an HFA type
if (StructHasCustomLayout(typeFlags) && compiler->IsHfa(typeHnd))
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to delete or replace the other use StructHasCustomLayout as well?

The CORINFO_FLG_CUSTOMLAYOUT flag on JIT/EE interface has always been very poorly defined. It would be nice to get rid of it, and replace it with well-defined type property or type properties as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it does. The only other user of StructHasCustomLayout is

if (StructHasCustomLayout(typeFlags) && ((typeFlags & CORINFO_FLG_CONTAINS_GC_PTR) == 0))
and based on the comment above the intent was to recognize structs with StructLayout(LayoutKind.Explicit, Size=...). However, in my local testing I found that structs marked with StructLayout(LayoutKind.Auto) also returns true for StructHasCustomLayout (e.g. ValueTuple).

Let me do more debugging here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been looking at the this and it seems that the original intent for StructHasCustomLayout was to identify LayoutKind.Explicit and prevent such structures from promoting (i.e. enregistering their fields).

I believe this is done to be able to properly copy such structs with explicitlayout attribute (incl. gaps between fields).

However, the way this is implemented in VM has interesting consequences.

For example, suppose we have

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace Runtime_64863
{
    class Program
    {
        struct DefLayout
        {
            ulong x0;
            uint w1;
        }

        [StructLayout(LayoutKind.Auto)]
        struct AutoLayout
        {
            ulong x0;
            uint w1;
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        static AutoLayout RetAutoLayout() => default(AutoLayout);

        [MethodImpl(MethodImplOptions.NoInlining)]
        static DefLayout RetDefLayout() => default(DefLayout);

        static void Main(string[] args)
        {
            RetAutoLayout();
            RetDefLayout();
        }
    }
}

On Arm64 DefLayout return type would be promotable

; Assembly listing for method Runtime_64863.Program:RetDefLayout():DefLayout
; Emitting BLENDED_CODE for generic ARM64 CPU - Windows
; optimized code
; fp based frame
; partially interruptible
; No PGO data
; Final local variable assignments
;
;* V00 loc0         [V00    ] (  0,  0   )  struct (16) zero-ref    multireg-ret ld-addr-op
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [sp+00H]   "OutgoingArgSpace"
;  V02 tmp1         [V02,T00] (  2,  2   )    long  ->   x0         single-def V00.x0(offs=0x00) P-INDEP "field V00.x0 (fldOffset=0x0)"
;  V03 tmp2         [V03,T01] (  2,  2   )     int  ->   x1         single-def V00.w1(offs=0x08) P-INDEP "field V00.w1 (fldOffset=0x8)"
;
; Lcl frame size = 0

G_M26462_IG01:              ;; offset=0000H
        A9BF7BFD          stp     fp, lr, [sp,#-16]!
        910003FD          mov     fp, sp
                                                ;; bbWeight=1    PerfScore 1.50
G_M26462_IG02:              ;; offset=0008H
        AA1F03E0          mov     x0, xzr
        2A1F03E1          mov     w1, wzr
                                                ;; bbWeight=1    PerfScore 1.00
G_M26462_IG03:              ;; offset=0010H
        A8C17BFD          ldp     fp, lr, [sp],#16
        D65F03C0          ret     lr
                                                ;; bbWeight=1    PerfScore 2.00

while AutoLayout return type would not:

; Assembly listing for method Runtime_64863.Program:RetAutoLayout():AutoLayout
; Emitting BLENDED_CODE for generic ARM64 CPU - Windows
; optimized code
; fp based frame
; partially interruptible
; No PGO data
; Final local variable assignments
;
;  V00 loc0         [V00,T00] (  2,  2   )  struct (16) [fp+10H]   do-not-enreg[SR] multireg-ret ld-addr-op
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [sp+00H]   "OutgoingArgSpace"
;
; Lcl frame size = 16

G_M39902_IG01:              ;; offset=0000H
        A9BE7BFD          stp     fp, lr, [sp,#-32]!
        910003FD          mov     fp, sp
                                                ;; bbWeight=1    PerfScore 1.50
G_M39902_IG02:              ;; offset=0008H
        4E040FF0          dup     v16.4s, wzr
        3D8007B0          str     q16, [fp,#16] // [V00 loc0]
        F9400BA0          ldr     x0, [fp,#16]  // [V00 loc0]
        F9400FA1          ldr     x1, [fp,#24]  // [V00 loc0+0x08]
                                                ;; bbWeight=1    PerfScore 7.00
G_M39902_IG03:              ;; offset=0018H
        A8C27BFD          ldp     fp, lr, [sp],#32
        D65F03C0          ret     lr
                                                ;; bbWeight=1    PerfScore 2.00

Maybe, I misunderstand what ECMA-335 says but isn't [StructLayout(LayoutKind.Auto)] implied by default?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, I misunderstand what ECMA-335 says but isn't [StructLayout(LayoutKind.Auto)] implied by default?

Wrt ECMA-335, there is always a StructLayout value written in metadata. This value is not optional.

The C# default for this value is StructLayout.Sequential for structs. So C# writes StructLayout.Sequential into metadata by default, unless it gets overridden by StructLayout attribute.

However, the way this is implemented in VM has interesting consequences.

Feel free to adjust the VM and JIT contract as needed. The existing CUSTOMLAYOUT flag is not very well defined. It would be great if we get something that well defined and easy to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I guess I confused the default value per ECMA

A class marked autolayout indicates that the loader is free to lay out the
class in any way it sees fit; any layout information that might have been specified is
ignored. This is the default.

and what C# and other compilers specify https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.layoutkind?view=net-6.0.

To reduce layout-related problems associated with the Auto value, C#, Visual Basic, and C++ compilers specify Sequential layout for value types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, the JIT primarily checks for combination of lvContainsHoles && lvCustomLayout in the code - looks that the intent was to check "a value type is not tightly packed and the "holes" are due to the layout being explicit". The assumption is that the "holes" can contain some information? If the "holes" are due to padding then the JIT can discard the information (as in DefLayout case in my example).

Looking at another example

struct DefLayout2
{
    uint w0;
    ulong x1;
}

this seems to be the case

; Assembly listing for method Runtime_64863.Program:RetDefLayout2():DefLayout2
; Emitting BLENDED_CODE for generic ARM64 CPU - Windows
; optimized code
; fp based frame
; partially interruptible
; No PGO data
; Final local variable assignments
;
;* V00 loc0         [V00    ] (  0,  0   )  struct (16) zero-ref    multireg-ret ld-addr-op
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [sp+00H]   "OutgoingArgSpace"
;  V02 tmp1         [V02,T00] (  2,  2   )     int  ->   x0         single-def V00.w0(offs=0x00) P-INDEP "field V00.w0 (fldOffset=0x0)"
;  V03 tmp2         [V03,T01] (  2,  2   )    long  ->   x1         single-def V00.x1(offs=0x08) P-INDEP "field V00.x1 (fldOffset=0x8)"
;
; Lcl frame size = 0

G_M35870_IG01:              ;; offset=0000H
        A9BF7BFD          stp     fp, lr, [sp,#-16]!
        910003FD          mov     fp, sp
                                                ;; bbWeight=1    PerfScore 1.50
G_M35870_IG02:              ;; offset=0008H
        2A1F03E0          mov     w0, wzr
        AA1F03E1          mov     x1, xzr
                                                ;; bbWeight=1    PerfScore 1.00
G_M35870_IG03:              ;; offset=0010H
        A8C17BFD          ldp     fp, lr, [sp],#16
        D65F03C0          ret     lr
                                                ;; bbWeight=1    PerfScore 2.00

; Total bytes of code 24, prolog size 8, PerfScore 6.90, instruction count 6, allocated bytes for code 24 (MethodHash=fc8373e1) for method Runtime_64863.Program:RetDefLayout2():DefLayout2
; ============================================================

if (TreatStructAsOpaqueBlobOfBytes(typeFlags))
{
structPromotionInfo.opaqueBlob = true;
return false;
}

Expand All @@ -1727,21 +1727,6 @@ bool Compiler::StructPromotionHelper::CanPromoteStructType(CORINFO_CLASS_HANDLE
unsigned structAlignment = roundUp(compHandle->getClassAlignmentRequirement(typeHnd), TARGET_POINTER_SIZE);
#endif // TARGET_ARM

// If we have "Custom Layout" then we might have an explicit Size attribute
// Managed C++ uses this for its structs, such C++ types will not contain GC pointers.
//
// The current VM implementation also incorrectly sets the CORINFO_FLG_CUSTOMLAYOUT
// whenever a managed value class contains any GC pointers.
// (See the comment for VMFLAG_NOT_TIGHTLY_PACKED in class.h)
//
// It is important to struct promote managed value classes that have GC pointers
// So we compute the correct value for "CustomLayout" here
//
if (StructHasCustomLayout(typeFlags) && ((typeFlags & CORINFO_FLG_CONTAINS_GC_PTR) == 0))
{
structPromotionInfo.customLayout = true;
}

if (StructHasDontDigFieldsFlagSet(typeFlags))
{
return CanConstructAndPromoteField(&structPromotionInfo);
Expand Down Expand Up @@ -1852,7 +1837,6 @@ bool Compiler::StructPromotionHelper::CanConstructAndPromoteField(lvaStructPromo
}

assert(!structPromotionInfo->containsHoles);
assert(!structPromotionInfo->customLayout);
lvaStructFieldInfo& fldInfo = structPromotionInfo->fields[0];

fldInfo.fldHnd = compHandle->getFieldInClass(typeHnd, 0);
Expand Down Expand Up @@ -1912,6 +1896,7 @@ bool Compiler::StructPromotionHelper::CanPromoteStructVar(unsigned lclNum)
assert(typeHnd != NO_CLASS_HANDLE);

bool canPromote = CanPromoteStructType(typeHnd);
varDsc->lvOpaqueBlob = structPromotionInfo.opaqueBlob;
if (canPromote && varDsc->lvIsMultiRegArgOrRet())
{
unsigned fieldCnt = structPromotionInfo.fieldCnt;
Expand Down Expand Up @@ -2030,11 +2015,6 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum)
structPromotionInfo.fieldCnt, varDsc->lvFieldAccessed);
shouldPromote = false;
}
else if (varDsc->lvIsMultiRegRet && structPromotionInfo.containsHoles && structPromotionInfo.customLayout)
{
JITDUMP("Not promoting multi-reg returned struct local V%02u with holes.\n", lclNum);
shouldPromote = false;
}
#if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_ARM)
// TODO-PERF - Only do this when the LclVar is used in an argument context
// TODO-ARM64 - HFA support should also eliminate the need for this.
Expand All @@ -2060,13 +2040,8 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum)
// multiple registers?
if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs))
{
if (structPromotionInfo.containsHoles && structPromotionInfo.customLayout)
{
JITDUMP("Not promoting multi-reg struct local V%02u with holes.\n", lclNum);
shouldPromote = false;
}
else if ((structPromotionInfo.fieldCnt != 2) &&
!((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType)))
if ((structPromotionInfo.fieldCnt != 2) &&
!((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType)))
{
JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields != 2 and it's "
"not a single SIMD.\n",
Expand Down Expand Up @@ -2297,7 +2272,6 @@ void Compiler::StructPromotionHelper::PromoteStructVar(unsigned lclNum)
varDsc->lvFieldLclStart = compiler->lvaCount;
varDsc->lvPromoted = true;
varDsc->lvContainsHoles = structPromotionInfo.containsHoles;
varDsc->lvCustomLayout = structPromotionInfo.customLayout;

#ifdef DEBUG
// Don't change the source to a TYP_BLK either.
Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/jit/liveness.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1718,9 +1718,8 @@ bool Compiler::fgComputeLifeUntrackedLocal(VARSET_TP& life,
VARSET_TP keepAliveFields(VarSetOps::Intersection(this, fieldSet, keepAliveVars));
noway_assert(VarSetOps::IsEmpty(this, keepAliveFields));

// Do not consider this store dead if the parent local variable is an address exposed local or
// if the struct has a custom layout and holes.
return !(varDsc.IsAddressExposed() || (varDsc.lvCustomLayout && varDsc.lvContainsHoles));
// Do not consider this store dead if the parent local variable is an address exposed.
return !varDsc.IsAddressExposed();
}
}
return false;
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10249,13 +10249,6 @@ GenTree* Compiler::fgMorphPromoteLocalInitBlock(GenTreeLclVar* destLclNode, GenT
return nullptr;
}

if (destLclVar->lvCustomLayout && destLclVar->lvContainsHoles)
{
// TODO-1stClassStructs: there are no reasons for this pessimization, delete it.
JITDUMP(" dest has custom layout and contains holes.\n");
return nullptr;
}

if (destLclVar->lvExactSize != blockSize)
{
JITDUMP(" dest size mismatch.\n");
Expand Down Expand Up @@ -17683,7 +17676,6 @@ void Compiler::fgRetypeImplicitByRefArgs()
newVarDsc->lvFieldLclStart = varDsc->lvFieldLclStart;
newVarDsc->lvFieldCnt = varDsc->lvFieldCnt;
newVarDsc->lvContainsHoles = varDsc->lvContainsHoles;
newVarDsc->lvCustomLayout = varDsc->lvCustomLayout;
#ifdef DEBUG
newVarDsc->lvKeepType = true;
#endif // DEBUG
Expand Down
10 changes: 2 additions & 8 deletions src/coreclr/jit/morphblock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -764,19 +764,13 @@ void MorphCopyBlockHelper::MorphStructCases()
requiresCopyBlock = true;
}

// Can we use field by field assignment for the dest?
if (m_dstDoFldAsg && m_dstVarDsc->lvCustomLayout && m_dstVarDsc->lvContainsHoles)
if (m_dstVarDsc && m_dstVarDsc->lvOpaqueBlob)
{
JITDUMP(" dest contains custom layout and contains holes");
// C++ style CopyBlock with holes
requiresCopyBlock = true;
}

// Can we use field by field assignment for the src?
if (m_srcDoFldAsg && m_srcVarDsc->lvCustomLayout && m_srcVarDsc->lvContainsHoles)
if (m_srcVarDsc && m_srcVarDsc->lvOpaqueBlob)
{
JITDUMP(" src contains custom layout and contains holes");
// C++ style CopyBlock with holes
requiresCopyBlock = true;
}

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1986,8 +1986,8 @@ private uint getClassAttribsInternal(TypeDesc type)
result |= CorInfoFlag.CORINFO_FLG_BYREF_LIKE;

// 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;
if (metadataType.IsExplicitLayout || metadataType.IsWellKnownType(WellKnownType.TypedReference))
result |= CorInfoFlag.CORINFO_FLG_OPAQUE_BLOB;

if (metadataType.IsUnsafeValueType)
result |= CorInfoFlag.CORINFO_FLG_UNSAFE_VALUECLASS;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ public enum CorInfoFlag : uint
CORINFO_FLG_OVERLAPPING_FIELDS = 0x00100000, // struct or class has fields that overlap (aka union)
CORINFO_FLG_INTERFACE = 0x00200000, // it is an interface
CORINFO_FLG_DONT_DIG_FIELDS = 0x00400000, // don't try to ask about fields outside of AOT compilation version bubble
CORINFO_FLG_CUSTOMLAYOUT = 0x00800000, // does this struct have custom layout?
CORINFO_FLG_OPAQUE_BLOB = 0x00800000, // should this struct be treated as opaque blob of data?
CORINFO_FLG_CONTAINS_GC_PTR = 0x01000000, // does the class contain a gc ptr ?
CORINFO_FLG_DELEGATE = 0x02000000, // is this a subclass of delegate or multicast delegate ?
// CORINFO_FLG_UNUSED = 0x04000000,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ std::string SpmiDumpHelper::DumpCorInfoFlag(CorInfoFlag flags)
AddFlag(CORINFO_FLG_ARRAY);
AddFlag(CORINFO_FLG_OVERLAPPING_FIELDS);
AddFlag(CORINFO_FLG_INTERFACE);
AddFlag(CORINFO_FLG_CUSTOMLAYOUT);
AddFlag(CORINFO_FLG_OPAQUE_BLOB);
AddFlag(CORINFO_FLG_CONTAINS_GC_PTR);
AddFlag(CORINFO_FLG_DELEGATE);
AddFlag(CORINFO_FLG_BYREF_LIKE);
Expand Down
7 changes: 3 additions & 4 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3629,11 +3629,10 @@ uint32_t CEEInfo::getClassAttribsInternal (CORINFO_CLASS_HANDLE clsHnd)
if (pMT->IsByRefLike())
ret |= CORINFO_FLG_BYREF_LIKE;

if ((pClass->IsNotTightlyPacked() && (!pClass->IsManagedSequential() || pClass->HasExplicitSize())) ||
pMT == g_TypedReferenceMT ||
VMClsHnd.IsNativeValueType())
if ((pClass->IsNotTightlyPacked() && (pClass->HasExplicitSize() || pClass->HasExplicitFieldOffsetLayout())) ||
(pMT == g_TypedReferenceMT) || VMClsHnd.IsNativeValueType())
{
ret |= CORINFO_FLG_CUSTOMLAYOUT;
ret |= CORINFO_FLG_OPAQUE_BLOB;
}

if (pClass->IsUnsafeValueClass())
Expand Down