Fix SystemV AMD64 Explicit structure classification by jkoritzinsky · Pull Request #22041 · dotnet/coreclr

jkoritzinsky · 2019-01-17T18:36:37Z

Our current implementation of our SystemV classifier just bails out on explicit structures. This PR changes our classifier to assign SystemV eightbytes with an algorithm that accounts for overlapping fields and fields that span multiple eightbytes. It also fixes a variety of bugs in the classifier related to structs (we wouldn't advance onto the next field when classifying a value-type or class-type field).

Fixes #4195
Fixes #4307
Fixes #22016
Fixes #23150
Fixes #23150

jashook · 2019-01-17T18:37:18Z

/cc @dotnet/jit-contrib

jkoritzinsky · 2019-01-17T18:38:11Z

@dotnet-bot test Ubuntu x64 Checked Build and Test
@dotnet-bot test OSX10.12 x64 Debug Build and Test

jashook · 2019-01-17T18:43:01Z

@jkoritzinsky this may affect several tests which used to assume explicit layout forces structs to the stack. Off the top of my head this could effect the structABI test and the FastTailCallCandidates tests.

CarolEidt · 2019-01-17T19:13:54Z

@jashook - I don't think that this will impact the JIT's decision to force explicit layout structs to the stack. That decision is made independent of how the struct is passed. That is, even if the struct is passed in registers, the JIT will already have determine that such structs can't be enregistered, so they would be loaded into the appropriate registers from the stack (or other memory location).

janvorli

Overall it looks good, but I still need to spend some more time looking at the classification code to get confident about it.

jashook · 2019-01-17T19:31:27Z

Looked into a few different tests, FastTailCallCandidates is effected as it used to attempt to test FastTailCalls with a caller/callee with stack space passing explicit structs. I believe there should not be an issue as the test still correct and mostly testing the same stuff.

jkotas · 2019-01-17T21:57:05Z

This is Unix x64 ABI change and it would require major R2R format increment.

cc @fadimounir @trylek

trylek · 2019-01-17T21:59:27Z

Hmm, and a port to the CPAOT GC ref map emitter :-).

jkoritzinsky · 2019-01-17T22:58:59Z

@dotnet-bot test Ubuntu x64 Checked Build and Test
@dotnet-bot test OSX10.12 x64 Debug Build and Test

jkoritzinsky · 2019-01-17T23:04:25Z

@dotnet-bot test Ubuntu x64 Checked Build and Test
@dotnet-bot test OSX10.12 x64 Debug Build and Test

jkotas · 2019-01-17T23:13:48Z

How is what we are doing here compatible with Mono? Is Mono going to be able to match this behavior?

cc @marek-safar

jkoritzinsky · 2019-01-17T23:35:18Z

As far as I can tell from a glance Mono's implementation, they're already fully compatible with the SystemV ABI here. So this change brings us more in line with Mono.

See https://github.com/mono/mono/blob/21d77b319a346f3eca439814f151a29de55b906f/mono/mini/mini-amd64.c#L631 for the Mono implementation.

jkotas · 2019-01-17T23:37:12Z

I do not see any place where Mono looks at the Fixed attribute...

jkoritzinsky · 2019-01-17T23:38:55Z

Good point on the FixedBufferAttribute. That part I don't see in mono either.

marek-safar · 2019-01-18T10:10:25Z

@jkotas we know that we are already incompatible especially with behaviour around overlapping. I think we will be able to match if it will be well defined or at least specified by test suite

jkotas · 2019-01-18T18:58:31Z

how the HFA stuff works

It is simpler version of the SystemV calling convention - it just deals with homegenous floating point values. E.g. consider this:

[StructLayout(LayoutKind.Explicit, Size = 8)]
struct MyHFA
{
    [Offset(0)]
    float f1;
    [Offset(4)]
    float f2;
}

And matching C/C++ structure like this:

struct MyHFA
{
    float f1;
    float f2;
}

Today, it does not work on either Unix x64 or any ARM platform. Your fix makes it work on Unix x64. I think we should make it work on ARM/ARM64 as well for consistency.

jkoritzinsky · 2019-01-18T21:58:25Z

@jkotas I've added support for aligned, non overlapping HFAs even if marked as explicit. I'm in the process of adding tests for it. I have a question though: Are structures with overlapping (float/double only and aligned) fields also allowed to be HFAs/not forbidden from being HFAs?

jkotas · 2019-01-18T22:01:46Z

Are structures with overlapping (float/double only and aligned) fields also allowed to be HFAs

I do not know. Try to compile a couple of tests using C++ compiler and look at the disassembly.

…istency.

jkotas · 2019-04-02T18:59:12Z

Also explicitly opt-out of HFAs for System.Numerics.Vector`1

We really want to be passing these perf-sensitive types in registers. Is this going be a perf regression from where we are today?

jkoritzinsky · 2019-04-02T19:03:09Z

The JIT classifies the System.Numerics.Vector<T> type somewhere else and actually requires that the SystemV and HFA classifiers don't accidentally capture them. The current code on master actually already disqualifies System.Numerics.Vector<T> from being an HFA by the fact that the System.Numerics.Register type is explicit-layout, so this doesn't actually change anything; it's preserving current behavior.

jkoritzinsky · 2019-04-02T23:21:21Z

/azp run coreclr-outerloop

azure-pipelines · 2019-04-02T23:21:36Z

Azure Pipelines successfully started running 1 pipeline(s).

jkoritzinsky · 2019-04-03T22:09:56Z

/azp run coreclr-outerloop

azure-pipelines · 2019-04-03T22:10:13Z

Azure Pipelines successfully started running 1 pipeline(s).

jkoritzinsky · 2019-04-04T17:28:05Z

/azp run coreclr-outerloop

azure-pipelines · 2019-04-04T17:28:15Z

Azure Pipelines failed to run 1 pipeline(s).

…mv-explicit

jkoritzinsky · 2019-04-04T17:29:39Z

/azp run coreclr-outerloop

azure-pipelines · 2019-04-04T17:29:53Z

Azure Pipelines successfully started running 1 pipeline(s).

jkoritzinsky · 2019-04-05T01:37:39Z

CoreFX failures are known and will be fixed by #23697.

CoreCLR Pri 1 failures are known and will be fixed by #23738

…2041) * Don't bail out on enregistering explicit structs if there are no overlapping fields. * Don't enregister if any unaligned fields. * Enable passing explicit structs by-value by enregistering on systemv. Some edge cases are likely still broken, but just removing our blanket opt-out makes the current tests pass. * Enable MarshalstructAsLayoutExp off-Windows. * Start adding additional tests for explicit layout to try to catch edge cases in SystemV classification. * Added a test that spans across multiple eightbytes and has an overlap in the second eightbyte. * Change repro to use an array of floats and an int field in managed and use a float array for padding in native to force an SSE classification on the first byte. * New algorithm to calculate eightbyte classification by going throw the structure byte-by-byte instead of field-by-field. * Fix updating eightbyte classifications in the loop to actually used the iterated-upon variable. * Consider each element of a fixed array as a separate field (to match native implementations). * Implement correct SystemV classification for fixed buffers in non-blittable structures. Fixed buffers in blittable structures have the managed layout assign classifications, which still is buggy. * Add tests. * Correctly classify blittable fixed buffers. Move "is this field a fixed buffer" tracking into one of the unused bits in FieldDesc as code that isn't in marshalers needs to know about it. * Handle the case where we have a struct that has no fields in an eightbyte that contains (i.e. no fields in the first eight bytes of the structure). * PR feedback. * Only look up FixedBufferAttribute when the type is a value class and the type of the field is a value type. * Use heuristic to determine if a type is a fixed buffer for SystemV classification. * Revert tracking if a field is a fixed buffer in the FieldDesc. * Update comments. * Classify aligned, nonoverlapping, float/double only structures as HFAs even if explicitly laid out * Enable overlapping fields in HFAs. Update NativeType HFA to check for alignment. I checked Godbolt to verify that HFAs for overlapping fields are allowed. * Add HFA tests. * Fix compile errors from HFA alignment check. * Non-valuetypes will never have their managed layout used to classify SystemV eightbytes. * Don't classify a struct with no zero-offset field as an HFA. * Remove duplicate semicolon. * PR feedback. * Add test with 2-field double HFA. * Clean up and add static asserts for struct size. * Add define for static_assert_no_msg to the native test headers * Fix build breaks. * Remove unneeded "size = X bytes" comments. They were holdovers from the .NET Framework test tree. * Use GetNumInstanceFieldBytes instead of GetLayoutInfo()->GetManagedSize() * Fix build break. * Centralize FieldMarshaler offsettting in ClassifyEightBytesWithNativeLayout. * Fix signed/unsigned mismatch * Fix condition to also detect arm64. * Change ifdef to if defined. * Remove duplicate declaration (broken in rebase) * Add some logging in one of the unreproable OSX test failures. * Mark System.Numerics.Vector as intrinsic and don't use the eightbyte classifier to enregister it. * Also explicitly opt-out of HFAs for System.Numerics.Vector`1 for consistency. * Update R2R required version to 3.0. * Remove debugging prints. Commit migrated from dotnet/coreclr@df80427

jkoritzinsky added the area-Interop label Jan 17, 2019

jkoritzinsky added this to the Future milestone Jan 17, 2019

jkoritzinsky requested review from AaronRobinsonMSFT, janvorli and jashook January 17, 2019 18:36

janvorli reviewed Jan 17, 2019

View reviewed changes

Comment thread src/vm/methodtable.cpp

Comment thread src/vm/methodtable.cpp Outdated

Comment thread src/vm/methodtable.cpp Outdated

tannergooding reviewed Jan 17, 2019

View reviewed changes

Comment thread src/vm/methodtablebuilder.cpp Outdated

jkotas reviewed Jan 17, 2019

View reviewed changes

Comment thread src/vm/methodtablebuilder.cpp Outdated