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.

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

Implement swift lowering algorithm in Mono's type system #99439

Merged
merged 11 commits into from
Mar 28, 2024

Conversation

jkoritzinsky
Copy link
Member

This implements the same algorithm as #99438 in the Mono type system.

This doesn't hook the APIs up to any of the codegen backends yet. I still need to figure out the best way to do that (suggestions welcome!).

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

  1. generic struct instances aren't MONO_TYPE_VALUETYPE
  2. i'm a bit concerned that we'll do a lot of throwaway temporary allocations for pathological cases. I wonder if we can bail out early when we can tell that lowering some struct will result in failure.
  3. it seems like some of the complexity here is to handle explicit overlapping layout. Is that the right intuition?

src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved
Comment on lines 6708 to 6709
GArray* lowered_bytes = g_array_sized_new(FALSE, TRUE, sizeof(SwiftPhysicalLoweringKind), m_class_get_instance_size(klass));

Copy link
Member

@lambdageek lambdageek Mar 8, 2024

Choose a reason for hiding this comment

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

Do we really need to account for every byte of the struct, or just the first 4*PointerSize bytes?
Can't we abort this whole algorithm if we're ever certain we're out of room?

Wonder if we can stack alloc this array with a maximum size or else just bail out

Copy link
Member Author

Choose a reason for hiding this comment

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

As of now, 4*PointerSize is the max, but once we have SIMD support, the max goes up drastically (as we could have up to 4 256-byte vectors here).

I'm using a slightly different algorithm in NativeAOT that I could probably use here instead of matching the CoreCLR algorithm that doesn't allocate "struct size" bytes.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok. I'm not sure we need a completely different algorithm (I didn't look at the NativeAOT one yet, so I don't know how much work it would entail), just rule out cases that are "obviously too big" - whatever that will mean even with SIMD. i'm particularly (perhaps mistakenly) concerned about InlineArray since it's trivial to make something absolutely massive.

src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved
@jkoritzinsky
Copy link
Member Author

  1. generic struct instances aren't MONO_TYPE_VALUETYPE

Good to know! I'll update the checks to handle that correctly.

  1. i'm a bit concerned that we'll do a lot of throwaway temporary allocations for pathological cases. I wonder if we can bail out early when we can tell that lowering some struct will result in failure.

Yeah, we could put a maximum here for the scenarios we currently support on the "number of bytes" portion of the algorithm.

  1. it seems like some of the complexity here is to handle explicit overlapping layout. Is that the right intuition?

Actually, most of the logic here is just to get the lowering right when accounting for padding. Only the logic in set_lowering_range is necessary for explicit layout. It was just easier to write the algorithm to support explicit layout and just do it all right than to add in code blocking it on all platforms (especially as I built the NativeAOT implementation to reuse the same logic as explicit layout validation).

Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

Thanks a lot for implementing the Mono support.

I think to handle the lowering, we will need to modify function signature possibly at:

mono_metadata_parse_method_signature_full (MonoImage *m, MonoGenericContainer *container,

I'm not sure if it would be possible to handle this at get_call_info level because the lowering can change number of passed struct fields (e.g., 5-field struct can be lowered to 4 elements). Any other ideas how to connect the Swift struct lowering to the Mono runtime @vargaz @lambdageek ?

Edit. I believe that maybe the handling at call level might be a better approach than modifying the function signature.

Comment on lines +6665 to +6669
// Normalize pointer types to IntPtr and resolve generic classes.
// We don't need to care about specific pointer types at this ABI level.
if (type->type == MONO_TYPE_PTR || type->type == MONO_TYPE_FNPTR) {
type = m_class_get_byval_arg (mono_defaults.int_class);
}
Copy link
Member

@matouskozak matouskozak Mar 18, 2024

Choose a reason for hiding this comment

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

Why do we need to change the pointer types here? The lowering of MONO_TYPE_PTR and MONO_TYPE_FNPTR is handled further down in this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

We handle it here for pointer fields in structs. The case below only handles on entry (ie when the type passed in is a pointer type).

@matouskozak
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matouskozak
Copy link
Member

While experimenting with connection this PR to mini codegen I encountered some minor issues with this implementation. However, since this is basically "dead code" at the moment, I will merge this PR and address the issues in subsequent PR.

@matouskozak matouskozak merged commit 12f5464 into dotnet:main Mar 28, 2024
105 of 112 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants