Fix auto layout algorithm to compute structure alignment correctly #73738

davidwrighton · 2022-08-11T00:37:35Z

In particular:
1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field
2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8.
The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8)

In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime.

This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now.

Fixes #65281

- In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8. - The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8) In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime. - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now. Fixes dotnet#65281

trylek · 2022-08-11T08:22:37Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

                }
            }

-            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;


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

trylek

Looks good, thanks for making another iteration in untangling this non-trivial algorithm!

trylek · 2022-08-11T20:29:44Z

/azp run runtime-coreclr crossgen2-composite

azure-pipelines · 2022-08-11T20:29:55Z

Azure Pipelines successfully started running 1 pipeline(s).

trylek · 2022-08-11T20:30:10Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2022-08-11T20:30:24Z

Azure Pipelines successfully started running 1 pipeline(s).

tannergooding · 2022-08-12T04:31:53Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

+                    // 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


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*)?

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?

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.

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.

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

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.

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.

tannergooding · 2022-08-12T04:34:21Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

+                        if (fieldType.IsPrimitive || fieldType.IsEnum)
+                        {
+                            // Handle alignment of long/ulong/double on ARM32
+                            largestAlignmentRequired = LayoutInt.Max(context.Target.GetObjectAlignment(fieldSizeAndAlignment.Size), largestAlignmentRequired);
+                        }


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.

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.

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?

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.

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.

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.

…_65281

danmoseley · 2022-08-17T02:21:21Z

.NET 7 backport candidate?

davidwrighton · 2022-08-17T18:50:20Z

/backport to release/7.0-rc1

github-actions · 2022-08-17T18:50:35Z

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2877629514

davidwrighton added 4 commits August 10, 2022 14:03

New test

74935a1

Re-enable disabled test

e599574

Remove file that shouldn't be added as part of the new test

3ef8ee4

davidwrighton requested a review from MichalStrehovsky as a code owner August 11, 2022 00:37

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Aug 11, 2022

davidwrighton requested a review from trylek August 11, 2022 00:37

ghost assigned davidwrighton Aug 11, 2022

trylek reviewed Aug 11, 2022

View reviewed changes

trylek approved these changes Aug 11, 2022

View reviewed changes

runfoapp bot mentioned this pull request Aug 11, 2022

Infra improvements for Helix #68176

Closed

Make a few test types public to silence unassigned field errors

0cdf40b

tannergooding reviewed Aug 12, 2022

View reviewed changes

davidwrighton added 2 commits August 16, 2022 10:13

Merge branch 'fix_65281' of github.com:davidwrighton/runtime into fix…

ebb920c

…_65281

Update comments and add more testing

174590f

davidwrighton merged commit 2fc2241 into dotnet:main Aug 17, 2022

github-actions bot mentioned this pull request Aug 17, 2022

[release/7.0-rc1] Fix auto layout algorithm to compute structure alignment correctly #74091

Merged

AaronRobinsonMSFT mentioned this pull request Aug 22, 2022

Test failure Interop/PInvoke/Int128/Int128TestFieldLayout/Int128TestFieldLayout.sh #74318

Closed

ghost locked as resolved and limited conversation to collaborators Sep 16, 2022

davidwrighton deleted the fix_65281 branch April 13, 2023 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix auto layout algorithm to compute structure alignment correctly #73738

Fix auto layout algorithm to compute structure alignment correctly #73738

davidwrighton commented Aug 11, 2022

trylek Aug 11, 2022

trylek left a comment

trylek commented Aug 11, 2022

azure-pipelines bot commented Aug 11, 2022

trylek commented Aug 11, 2022

azure-pipelines bot commented Aug 11, 2022

tannergooding Aug 12, 2022

trylek Aug 12, 2022

tannergooding Aug 12, 2022

davidwrighton Aug 16, 2022

davidwrighton Aug 16, 2022

tannergooding Aug 16, 2022

davidwrighton Aug 16, 2022

tannergooding Aug 12, 2022 •

edited

Loading

davidwrighton Aug 16, 2022

tannergooding Aug 16, 2022

davidwrighton Aug 16, 2022

davidwrighton Aug 16, 2022

davidwrighton Aug 16, 2022

danmoseley commented Aug 17, 2022

davidwrighton commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

Fix auto layout algorithm to compute structure alignment correctly #73738

Fix auto layout algorithm to compute structure alignment correctly #73738

Conversation

davidwrighton commented Aug 11, 2022

Choose a reason for hiding this comment

trylek left a comment

Choose a reason for hiding this comment

trylek commented Aug 11, 2022

azure-pipelines bot commented Aug 11, 2022

trylek commented Aug 11, 2022

azure-pipelines bot commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Aug 17, 2022

davidwrighton commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

tannergooding Aug 12, 2022 •

edited

Loading