Implement swift lowering algorithm in Mono's type system#99439
Implement swift lowering algorithm in Mono's type system#99439matouskozak merged 11 commits intodotnet:mainfrom
Conversation
lambdageek
left a comment
There was a problem hiding this comment.
- generic struct instances aren't MONO_TYPE_VALUETYPE
- 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.
- 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
| GArray* lowered_bytes = g_array_sized_new(FALSE, TRUE, sizeof(SwiftPhysicalLoweringKind), m_class_get_instance_size(klass)); | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Good to know! I'll update the checks to handle that correctly.
Yeah, we could put a maximum here for the scenarios we currently support on the "number of bytes" portion of the algorithm.
Actually, most of the logic here is just to get the lowering right when accounting for padding. Only the logic in |
…he future when we support vectors.
There was a problem hiding this comment.
Thanks a lot for implementing the Mono support.
I think to handle the lowering, we will need to modify function signature possibly at:
runtime/src/mono/mono/metadata/metadata.c
Line 2638 in 7f4702a
I'm not sure if it would be possible to handle this at (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 ?get_call_info level because the lowering can change number of passed struct fields
Edit. I believe that maybe the handling at call level might be a better approach than modifying the function signature.
| // 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); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
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. |
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!).