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

[x64][SysV] Classify empty structs for passing like padding #103799

Merged
merged 4 commits into from
Jun 28, 2024
Merged

[x64][SysV] Classify empty structs for passing like padding #103799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aba87b6
Classify empty structs on x64 like padding
tomeksowi May 8, 2024
131a3c5
Add comment to fix JIT to AssignClassifiedEightByteTypes
tomeksowi Jun 21, 2024
cb22839
Add tests for empty struct field passing on System V
tomeksowi Jun 24, 2024
db406a7
Update field names in C++ tests to match C#
tomeksowi Jun 24, 2024
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
23 changes: 18 additions & 5 deletions src/coreclr/tools/Common/JitInterface/SystemVStructClassificator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using ILCompiler;
using Internal.TypeSystem;
using System.Runtime.CompilerServices;
using static Internal.JitInterface.SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR;
using static Internal.JitInterface.SystemVClassificationType;

Expand Down Expand Up @@ -207,7 +208,10 @@ private static bool ClassifyEightBytes(TypeDesc typeDesc,

if (numIntroducedFields == 0)
{
return false;
// Classify empty struct like padding
helper.LargestFieldOffset = startOffsetOfStruct;
AssignClassifiedEightByteTypes(ref helper);
return true;
}

// The SIMD and Int128 Intrinsic types are meant to be handled specially and should not be passed as struct registers
Expand Down Expand Up @@ -375,8 +379,12 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
// Calculate the eightbytes and their types.

int lastFieldOrdinal = sortedFieldOrder[largestFieldOffset];
int offsetAfterLastFieldByte = largestFieldOffset + helper.FieldSizes[lastFieldOrdinal];
SystemVClassificationType lastFieldClassification = helper.FieldClassifications[lastFieldOrdinal];
int lastFieldSize = (lastFieldOrdinal >= 0) ? helper.FieldSizes[lastFieldOrdinal] : 0;
int offsetAfterLastFieldByte = largestFieldOffset + lastFieldSize;
Debug.Assert(offsetAfterLastFieldByte <= helper.StructSize);
SystemVClassificationType lastFieldClassification = (lastFieldOrdinal >= 0)
? helper.FieldClassifications[lastFieldOrdinal]
: SystemVClassificationTypeNoClass;

int usedEightBytes = 0;
int accumulatedSizeForEightBytes = 0;
Expand All @@ -403,6 +411,8 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
// the SysV ABI spec.
fieldSize = 1;
fieldClassificationType = offset < offsetAfterLastFieldByte ? SystemVClassificationTypeNoClass : lastFieldClassification;
if (offset % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // new eightbyte
foundFieldInEightByte = false;
}
else
{
Expand Down Expand Up @@ -455,13 +465,16 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
}
}

if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // If we just finished checking the last byte of an eightbyte
// If we just finished checking the last byte of an eightbyte or the entire struct
if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0 || (offset + 1) == helper.StructSize)
{
if (!foundFieldInEightByte)
{
// If we didn't find a field in an eight-byte (i.e. there are no explicit offsets that start a field in this eightbyte)
// If we didn't find a field in an eightbyte (i.e. there are no explicit offsets that start a field in this eightbyte)
// then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT
// so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass.
//
// TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this.
Copy link
Member

Choose a reason for hiding this comment

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

What does the difference between NoClass and something like char[8] end up being? For significant padding (structs with explicit layout) it seems like we have to consider them to be the same or things will end up odd/wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

char[8] is an INTEGER eightbyte which gets assigned an integer register for passing; a NO_CLASS eightbyte doesn't get any register. Passing on the stack is the same, though (NO_CLASS padding does take up space).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #104098 as I'll need an URL for ActiveIssue in tests in future PRs.

Meanwhile this smaller fix is ready for review.

Copy link
Member

Choose a reason for hiding this comment

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

Does this fix handle the significant padding cases correctly (i.e. as before this change)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, the NO_CLASS -> INTEGER eightbyte reclassification is still there. And I didn't see any CLR tests (prio1) go off on Checked build, assuming there are tests for significant padding by now.

if (helper.EightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] == SystemVClassificationTypeNoClass)
{
helper.EightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] = SystemVClassificationTypeInteger;
Expand Down
35 changes: 26 additions & 9 deletions src/coreclr/vm/methodtable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2114,11 +2114,14 @@ bool MethodTable::ClassifyEightBytesWithManagedLayout(SystemVStructRegisterPassi

DWORD numIntroducedFields = GetNumIntroducedInstanceFields();

// It appears the VM gives a struct with no fields of size 1.
// Don't pass in register such structure.
if (numIntroducedFields == 0)
{
return false;
helperPtr->largestFieldOffset = startOffsetOfStruct;
LOG((LF_JIT, LL_EVERYTHING, "%*s**** Classify empty struct %s (%p) like padding, startOffset %d, total struct size %d\n",
nestingLevel * 5, "", this->GetDebugClassName(), this, startOffsetOfStruct, helperPtr->structSize));

AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
return true;
}

// The SIMD Intrinsic types are meant to be handled specially and should not be passed as struct registers
Expand Down Expand Up @@ -2334,7 +2337,12 @@ bool MethodTable::ClassifyEightBytesWithNativeLayout(SystemVStructRegisterPassin
// No fields.
if (numIntroducedFields == 0)
{
return false;
helperPtr->largestFieldOffset = startOffsetOfStruct;
LOG((LF_JIT, LL_EVERYTHING, "%*s**** Classify empty struct %s (%p) like padding, startOffset %d, total struct size %d\n",
nestingLevel * 5, "", this->GetDebugClassName(), this, startOffsetOfStruct, helperPtr->structSize));

AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
return true;
}

bool hasImpliedRepeatedFields = HasImpliedRepeatedFields(this);
Expand Down Expand Up @@ -2586,8 +2594,12 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
// Calculate the eightbytes and their types.

int lastFieldOrdinal = sortedFieldOrder[largestFieldOffset];
unsigned int offsetAfterLastFieldByte = largestFieldOffset + helperPtr->fieldSizes[lastFieldOrdinal];
SystemVClassificationType lastFieldClassification = helperPtr->fieldClassifications[lastFieldOrdinal];
unsigned int lastFieldSize = (lastFieldOrdinal >= 0) ? helperPtr->fieldSizes[lastFieldOrdinal] : 0;
unsigned int offsetAfterLastFieldByte = largestFieldOffset + lastFieldSize;
_ASSERTE(offsetAfterLastFieldByte <= helperPtr->structSize);
SystemVClassificationType lastFieldClassification = (lastFieldOrdinal >= 0)
? helperPtr->fieldClassifications[lastFieldOrdinal]
: SystemVClassificationTypeNoClass;

unsigned int usedEightBytes = 0;
unsigned int accumulatedSizeForEightBytes = 0;
Expand All @@ -2614,6 +2626,8 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
// the SysV ABI spec.
fieldSize = 1;
fieldClassificationType = offset < offsetAfterLastFieldByte ? SystemVClassificationTypeNoClass : lastFieldClassification;
if (offset % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // new eightbyte
foundFieldInEightByte = false;
}
else
{
Expand Down Expand Up @@ -2666,13 +2680,16 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
}
}

if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // If we just finished checking the last byte of an eightbyte
// If we just finished checking the last byte of an eightbyte or the entire struct
if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0 || (offset + 1) == helperPtr->structSize)
{
if (!foundFieldInEightByte)
{
// If we didn't find a field in an eight-byte (i.e. there are no explicit offsets that start a field in this eightbyte)
// If we didn't find a field in an eightbyte (i.e. there are no explicit offsets that start a field in this eightbyte)
// then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT
// so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass.
//
// TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this.
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to fix this TODO in this PR?

Copy link
Contributor Author

@tomeksowi tomeksowi Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No. It would require going through the JIT like I did in #101796 for RISC-V and fix in many places. Right now my priority is on RISC-V but when I'm done with #101796 I can revisit it. By then @jakobbotsch's ongoing effort to centralize ABI passing infos will probably be more advanced, which will also facilitate fixing x64.

if (helperPtr->eightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] == SystemVClassificationTypeNoClass)
{
helperPtr->eightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] = SystemVClassificationTypeInteger;
Expand Down Expand Up @@ -2705,9 +2722,9 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
LOG((LF_JIT, LL_EVERYTHING, " **** Number EightBytes: %d\n", helperPtr->eightByteCount));
for (unsigned i = 0; i < helperPtr->eightByteCount; i++)
{
_ASSERTE(helperPtr->eightByteClassifications[i] != SystemVClassificationTypeNoClass);
LOG((LF_JIT, LL_EVERYTHING, " **** eightByte %d -- classType: %s, eightByteOffset: %d, eightByteSize: %d\n",
i, GetSystemVClassificationTypeName(helperPtr->eightByteClassifications[i]), helperPtr->eightByteOffsets[i], helperPtr->eightByteSizes[i]));
_ASSERTE(helperPtr->eightByteClassifications[i] != SystemVClassificationTypeNoClass);
}
#endif // _DEBUG
}
Expand Down
Loading