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

Jump to bottom

Fix auto layout algorithm to compute structure alignment correctly #73738

Merged
merged 7 commits into from
Aug 17, 2022
Merged

Fix auto layout algorithm to compute structure alignment correctly #73738

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74935a1
New test
davidwrighton Aug 10, 2022
be730bc
Fix auto layout algorithm to compute structure alignment correctly
davidwrighton Aug 11, 2022
e599574
Re-enable disabled test
davidwrighton Aug 11, 2022
3ef8ee4
Remove file that shouldn't be added as part of the new test
davidwrighton Aug 11, 2022
0cdf40b
Make a few test types public to silence unassigned field errors
trylek Aug 11, 2022
ebb920c
Merge branch 'fix_65281' of github.com:davidwrighton/runtime into fix…
davidwrighton Aug 16, 2022
174590f
Update comments and add more testing
davidwrighton Aug 16, 2022
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
36 changes: 26 additions & 10 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,25 +520,41 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
if (!fieldLayoutAbiStable)
layoutAbiStable = false;

largestAlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequired);

if (IsByValueClass(fieldType))
{
largestAlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequired);
instanceValueClassFieldsArr[instanceValueClassFieldCount++] = field;
}
else if (fieldType.IsGCPointer)
{
instanceGCPointerFieldsArr[instanceGCPointerFieldsCount++] = field;
}
else
{
int log2size = CalculateLog2(fieldSizeAndAlignment.Size.AsInt);
instanceNonGCPointerFieldsArr[log2size][instanceNonGCPointerFieldsCount[log2size]++] = field;
// non-value-type fields always require pointer alignment
// This does not account for types that are marked IsAlign8Candidate due to 8-byte fields
// but that is explicitly handled when we calculate the final alignment for the type.

// This behavior is extremely strange for primitive types, as it makes a struct with a single byte in it
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this? Wouldn't struct S { byte x; } hit IsByValueClass(fieldType) above and so it would be 1-byte alignment?

Should this instead say class since it looks like class C { byte x; } would hit this case and it would be align: sizeof(void*)?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, these questions need David's response but he's on vacation next week. Should we hold off merging this change until he's back so that he can answer them?

Copy link
Member

Choose a reason for hiding this comment

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

Was this targeting .NET 7?

If so then for this one, its just a question on comment wording, I don't think it's worth blocking over.

For the one below, I think it's worth confirming that CG2 is at least matching what the RyuJIT VM does. If it is, then I think its fine to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment needs clarity, and possibly a test about this particularly odd behavior. From my reading, the rules are that:

struct S1
{
    byte b;
}
[StructLayout(LayoutKind.Auto)]
struct S1_Auto
{
    byte b;
}
struct S2
{
    S1 fld1;
    S1_Auto fld2;
}

Assuming 64bit...
This will result in S1 having size of 1, and alignment of 1, S1_Auto having size of 8 and alignment of 8, and S2 having size of 16, and alignment of 8. Note that this wacky behavior is exclusive to auto layout structures.

Largest alignment rules for classes are less interesting in general, and pretty much just impact ARM32, where a class alignment of 8 is significant, and triggers the 8 byte alignment behavior. However, we still calculate them, in case we add support for 16 byte or higher object alignment rules in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also IsByValueClass(t) is false for primitive and enum types, and so doesn't go into the structure alignment path.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this wacky behavior is exclusive to auto layout structures.

I thought we fixed some of this for generics so that Nullable<T> wasn't "too large" anymore? Or was that a different fix since its generic but sequential?

This, more generally, seems like something which may have undesirable consequences and which we may want to look at more in depth for .NET 8. Consider types like ValueTuple which are frequently used for tuples of primitives and where its marked Auto with the intent that the BCL optimizes the layout to be more efficient. If the alignment of fields is treated as 8, that is not going to be the case and many usages will end up more expensive instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm... maybe? I've been writing tests, and to get the wacky layout behavior, I need to make the auto layout structure be larger than a pointer, so the tests that cover these boundary conditions are even dumber than I thought.

// have 8 byte alignment, but that is the current implementation.

largestAlignmentRequired = LayoutInt.Max(new LayoutInt(context.Target.PointerSize), largestAlignmentRequired);

if (fieldType.IsGCPointer)
{
instanceGCPointerFieldsArr[instanceGCPointerFieldsCount++] = field;
}
else
{
int log2size = CalculateLog2(fieldSizeAndAlignment.Size.AsInt);
instanceNonGCPointerFieldsArr[log2size][instanceNonGCPointerFieldsCount[log2size]++] = field;

if (fieldType.IsPrimitive || fieldType.IsEnum)
{
// Handle alignment of long/ulong/double on ARM32
largestAlignmentRequired = LayoutInt.Max(context.Target.GetObjectAlignment(fieldSizeAndAlignment.Size), largestAlignmentRequired);
}
Comment on lines +555 to +559
Copy link
Member

@tannergooding tannergooding Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What about Vector128<T>? Don't we want that to have 16-byte packing (on supported platforms) even if the GC only guarantees 8-byte alignment?

That would also, seemingly, ensure we have "less changes" to do if the GC ever supports 16-byte alignment in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Vector128<T> uses a different layout algorithm to calculate its alignment. When used as a field of a structure, it will be 16byte aligned within the algorithm with the existing scheme which will result in 16byte alignment for the entire structure.

Copy link
Member

Choose a reason for hiding this comment

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

Right, struct S1 { byte x; Vector128<...> y; } and struct S2 { Vector128<...> x; byte y } should both be Pack = 16 and therefore size = 32. Therefore a struct containing S1 or S2 should likewise be Pack = 16 so that the S1/S2 field are at the correct offset.

So shouldn't this factor into largestAlignmentRequired? If not, could you explain the difference between this and the other algorithm and why they are distinct/separate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's all correct, and does funnel into this algorithm, but the portion you commented on is exclusively for primitives, enums, and pointers (of various types). Normal valuetypes, and the various vector things (and Int128, etc) are not part of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this is the auto layout algorithm, not the sequential layout algorithm. Note, in addition, I'm looking at adding some tests to look at the behavior of classes with fields of higher than pointer alignment, but we'll see where that goes. My suspicion is that the current model for 16 byte structures in class types, in the presence of derived types across R2R version boundaries, does not actually reliably result in aligned 16byte fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

But that is irrelevant to the thrust of these changes, which are about allowing higher alignment to affect auto layout fields at all within crossgen2.

}
}
}

largestAlignmentRequired = context.Target.GetObjectAlignment(largestAlignmentRequired);
bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && largestAlignmentRequired.AsInt > 4;
bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && context.Target.PointerSize == 4 && context.Target.GetObjectAlignment(largestAlignmentRequired).AsInt > 4 && context.Target.PointerSize == 4;
Copy link
Member

Choose a reason for hiding this comment

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

Nit - you're checking context.Target.PointerSize == 4 twice.


// For types inheriting from another type, field offsets continue on from where they left off
// Base alignment is not always required, it's only applied when there's a version bubble boundary
Expand Down
Loading