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

Add support for SwiftSelf<T> in Mono JIT and Interpreter #104171

Merged
merged 15 commits into from
Jul 19, 2024
Merged

Add support for SwiftSelf<T> in Mono JIT and Interpreter #104171

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0647f6a
Add support for SwiftSelf<T>
kotlarmilos Jun 28, 2024
3008c6c
Add constraints
kotlarmilos Jun 28, 2024
240c1b3
Enable runtime tests
kotlarmilos Jun 28, 2024
5bcb0a5
Add swift_self_t instead of strcmp check
kotlarmilos Jun 29, 2024
630e6a9
Use int_class instead of typed_reference
kotlarmilos Jun 29, 2024
a2c1d78
Remove redundant branch for SwiftSelf<T>
kotlarmilos Jul 9, 2024
e375636
Merge branch 'main' into feature/swift-self-generic
kotlarmilos Jul 9, 2024
692aacc
Update src/mono/mono/mini/mini-amd64.c
kotlarmilos Jul 12, 2024
5756236
Update src/mono/mono/mini/mini-arm64.c
kotlarmilos Jul 12, 2024
04eee19
Use sizeof(MonoObject) offset for swift_self_t
kotlarmilos Jul 12, 2024
0b05920
Merge branch 'main' into feature/swift-self-generic
kotlarmilos Jul 15, 2024
110534a
Use MONO_ABI_SIZEOF to load boxed struct as an argument
kotlarmilos Jul 15, 2024
b902f1d
Load the address of the struct with 0 offset from SwiftSelf<T>
kotlarmilos Jul 16, 2024
30c14b0
Resolve conflicts
kotlarmilos Jul 16, 2024
21f7fc7
Update NEW_ARGLOADA to the pointer-like type
kotlarmilos Jul 16, 2024
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
15 changes: 13 additions & 2 deletions src/mono/mono/metadata/marshal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,9 @@ static GENERATE_TRY_GET_CLASS_WITH_CACHE (suppress_gc_transition_attribute, "Sys
static GENERATE_TRY_GET_CLASS_WITH_CACHE (unmanaged_callers_only_attribute, "System.Runtime.InteropServices", "UnmanagedCallersOnlyAttribute")
static GENERATE_TRY_GET_CLASS_WITH_CACHE (unmanaged_callconv_attribute, "System.Runtime.InteropServices", "UnmanagedCallConvAttribute")

GENERATE_TRY_GET_CLASS_WITH_CACHE (swift_error, "System.Runtime.InteropServices.Swift", "SwiftError")
GENERATE_TRY_GET_CLASS_WITH_CACHE (swift_self, "System.Runtime.InteropServices.Swift", "SwiftSelf")
GENERATE_TRY_GET_CLASS_WITH_CACHE (swift_self_t, "System.Runtime.InteropServices.Swift", "SwiftSelf`1");
GENERATE_TRY_GET_CLASS_WITH_CACHE (swift_error, "System.Runtime.InteropServices.Swift", "SwiftError")
GENERATE_TRY_GET_CLASS_WITH_CACHE (swift_indirect_result, "System.Runtime.InteropServices.Swift", "SwiftIndirectResult")

static gboolean type_is_blittable (MonoType *type);
Expand Down Expand Up @@ -3698,20 +3699,30 @@ mono_marshal_get_native_wrapper (MonoMethod *method, gboolean check_exceptions,

if (mono_method_signature_has_ext_callconv (csig, MONO_EXT_CALLCONV_SWIFTCALL)) {
MonoClass *swift_self = mono_class_try_get_swift_self_class ();
MonoClass *swift_self_t = mono_class_try_get_swift_self_t_class ();
MonoClass *swift_error = mono_class_try_get_swift_error_class ();
MonoClass *swift_indirect_result = mono_class_try_get_swift_indirect_result_class ();
MonoClass *swift_error_ptr = mono_class_create_ptr (m_class_get_this_arg (swift_error));
int swift_error_args = 0, swift_self_args = 0, swift_indirect_result_args = 0;
for (int i = 0; i < method->signature->param_count; ++i) {
MonoClass *param_klass = mono_class_from_mono_type_internal (method->signature->params [i]);
MonoGenericClass *param_gklass = mono_class_try_get_generic_class (param_klass);
if (param_klass) {
if (param_klass == swift_error && !m_type_is_byref (method->signature->params [i])) {
swift_error_args = swift_self_args = 0;
mono_error_set_generic_error (emitted_error, "System", "InvalidProgramException", "SwiftError argument must be passed by reference.");
break;
} else if (param_klass == swift_error || param_klass == swift_error_ptr) {
swift_error_args++;
} else if (param_klass == swift_self) {
} else if (param_gklass && (param_gklass->container_class == swift_self_t) && i > 0) {
swift_error_args = swift_self_args = 0;
mono_error_set_generic_error (emitted_error, "System", "InvalidProgramException", "SwiftSelf<T> must be the first argument in the signature.");
break;
} else if (param_gklass && (param_gklass->container_class == swift_self_t) && m_type_is_byref (method->signature->params [i])) {
swift_error_args = swift_self_args = 0;
mono_error_set_generic_error (emitted_error, "System", "InvalidProgramException", "Expected SwiftSelf<T> struct, got pointer/reference.");
break;
} else if (param_klass == swift_self || (param_gklass && (param_gklass->container_class == swift_self_t))) {
swift_self_args++;
} else if (param_klass == swift_indirect_result) {
swift_indirect_result_args++;
Expand Down
1 change: 1 addition & 0 deletions src/mono/mono/metadata/marshal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,7 @@ IlgenCallbacksToMono*
mono_marshal_get_mono_callbacks_for_ilgen (void);

GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL (swift_self)
GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL (swift_self_t);
GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL (swift_error)
GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL (swift_indirect_result)

Expand Down
8 changes: 7 additions & 1 deletion src/mono/mono/mini/interp/transform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3406,6 +3406,7 @@ interp_emit_swiftcall_struct_lowering (TransformData *td, MonoMethodSignature *c
uint32_t new_param_count = 0;
int align;
MonoClass *swift_self = mono_class_try_get_swift_self_class ();
MonoClass *swift_self_t = mono_class_try_get_swift_self_t_class ();
MonoClass *swift_error = mono_class_try_get_swift_error_class ();
MonoClass *swift_indirect_result = mono_class_try_get_swift_indirect_result_class ();
/*
Expand All @@ -3416,6 +3417,8 @@ interp_emit_swiftcall_struct_lowering (TransformData *td, MonoMethodSignature *c
for (int idx_param = 0; idx_param < csignature->param_count; ++idx_param) {
MonoType *ptype = csignature->params [idx_param];
MonoClass *klass = mono_class_from_mono_type_internal (ptype);
MonoGenericClass *gklass = mono_class_try_get_generic_class (klass);

// SwiftSelf, SwiftError, and SwiftIndirectResult are special cases where we need to preserve the class information for the codegen to handle them correctly.
if (mono_type_is_struct (ptype) && !(klass == swift_self || klass == swift_error || klass == swift_indirect_result)) {
SwiftPhysicalLowering lowered_swift_struct = mono_marshal_get_swift_physical_lowering (ptype, FALSE);
Copy link
Member

Choose a reason for hiding this comment

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

Does this lower as SwiftSelf<T> instead of as the inner element type? That might be problematic since the layout of SwiftSelf<T> may not match the layout of T due to potentially added padding at the end.

Copy link
Member

Choose a reason for hiding this comment

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

I think you are right, it seems we are lowering the generic, not the inner type. Could you share more details about when padding would be added?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referring that it should first retrieve the inner type if klass is SwiftSelf<T>, and then perform the lowering?

Copy link
Member

Choose a reason for hiding this comment

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

Could you share more details about when padding would be added?

For example, the Swift struct

struct Foo {
  let A : Int32
  let B : UInt8
}

corresponds to the C# struct

[StructLayout(LayoutKind.Sequential, Pack = 1)]
struct Foo
{
  public int A;
  public byte B;
}

But we have
Foo - size 5
SwiftSelf<Foo> - size 8

Does this actually matter for the final lowering and ABI? Not totally sure when "self" is the last argument, actually. I wouldn't be surprised if there are cases where lowering SwiftSelf<Foo> instead of Foo ends up with some different register assignments.

Are you referring that it should first retrieve the inner type if klass is SwiftSelf<T>, and then perform the lowering?

Right.

Expand All @@ -3437,7 +3440,10 @@ interp_emit_swiftcall_struct_lowering (TransformData *td, MonoMethodSignature *c
}
} else {
// For structs that cannot be lowered, we change the argument to byref type
ptype = mono_class_get_byref_type (mono_defaults.typed_reference_class);
if (gklass && (gklass->container_class == swift_self_t))
ptype = mono_class_get_byref_type (swift_self);
else
ptype = mono_class_get_byref_type (mono_defaults.int_class);
Copy link
Member

Choose a reason for hiding this comment

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

As with other comments, don't we just care here that the type is a simple pointer type? Why are we using a byref of a class that doesn't represents the actual contents. Shouldn't we have here, both in case of SwiftSelf and SwiftSelf<T>, the type being just m_class_get_byval_arg (mono_defaults.int_class), aka a naked pointer passed by value.

Copy link
Member Author

Choose a reason for hiding this comment

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

If SwiftSelf<T> can't be lowered, then it should be passed in the same manner as SwiftSelf, via the context register. This ensures that the get_call_info allocates the correct register, instead of any argument register.

// Load the address of the struct
interp_add_ins (td, MINT_LDLOCA_S);
interp_ins_set_sreg (td->last_ins, sp_old_params [idx_param].var);
Expand Down
19 changes: 17 additions & 2 deletions src/mono/mono/mini/method-to-ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7533,6 +7533,7 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
GArray *new_params = g_array_sized_new (FALSE, FALSE, sizeof (MonoType*), n);
uint32_t new_param_count = 0;
MonoClass *swift_self = mono_class_try_get_swift_self_class ();
MonoClass *swift_self_t = mono_class_try_get_swift_self_t_class ();
MonoClass *swift_error = mono_class_try_get_swift_error_class ();
MonoClass *swift_indirect_result = mono_class_try_get_swift_indirect_result_class ();
/*
Expand All @@ -7543,6 +7544,7 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
for (int idx_param = 0; idx_param < n; ++idx_param) {
MonoType *ptype = fsig->params [idx_param];
MonoClass *klass = mono_class_from_mono_type_internal (ptype);
MonoGenericClass *gklass = mono_class_try_get_generic_class (klass);

// SwiftSelf, SwiftError, and SwiftIndirectResult are special cases where we need to preserve the class information for the codegen to handle them correctly.
if (mono_type_is_struct (ptype) && !(klass == swift_self || klass == swift_error || klass == swift_indirect_result)) {
Expand All @@ -7565,12 +7567,25 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
}
} else {
// For structs that cannot be lowered, we change the argument to byref type
ptype = mono_class_get_byref_type (mono_defaults.typed_reference_class);
// Load the address of the struct
if (gklass && (gklass->container_class == swift_self_t))
ptype = mono_class_get_byref_type (swift_self);
else
ptype = mono_class_get_byref_type (mono_defaults.int_class);
Copy link
Member

Choose a reason for hiding this comment

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

Why is it mono_class_get_byref_type (mono_defaults.int_class) instead of mono_class_get_byref_type (mono_defaults.typed_reference_class) or mono_class_get_byref_type (klass); ?

Copy link
Member Author

Choose a reason for hiding this comment

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

#104171 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@jkurdek TypedReference& is kind of weird since it's a ref of a byreflike struct. I don't remember if we support that in .NET yet. it might make Mono do something weird.

mono_class_get_byref_type (klass) might be ok.

@kotlarmilos I think it's probably worth adding some comment about the placeholder type here - it's arbitrary and not really meant to exactly capture the exact type - all we care about is that it's some kind of pointer-like argument.


MonoInst *struct_base_address = mono_compile_create_var (cfg, mono_get_int_type (), OP_LOCAL);
CHECK_ARG (idx_param);
NEW_ARGLOADA (cfg, struct_base_address, idx_param);
MONO_ADD_INS (cfg->cbb, struct_base_address);

// Load the address of the struct
if (gklass && (gklass->container_class == swift_self_t))
{
MonoClassField *klass_fields = m_class_get_fields (klass);
MonoInst *swift_self_base_address = struct_base_address;
struct_base_address = mono_compile_create_var (cfg, mono_get_int_type (), OP_LOCAL);
MONO_EMIT_NEW_LOAD_MEMBASE_OP (cfg, OP_ADD_IMM, struct_base_address->dreg, swift_self_base_address->dreg, klass_fields->offset);
Copy link
Member

@BrzVlad BrzVlad Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't klass_fields->offset always be 0 ? Also I think it is offset with sizeof (MonoObject)

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, you are right. I was missing sizeof (MonoObject).

Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between sizeof (MonoObject) and MONO_ABI_SIZEOF (MonoObject)? I've been seeing more of the latter in when dealing with loading structs so I wonder if it's the same here...

Copy link
Member

Choose a reason for hiding this comment

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

I think the only difference is that MONO_ABI_SIZEOF (MonoObject) works with cross compiler. So if you are say on arm64 and you aot compile code for an arm target, sizeof (MonoObject) is 16 but in generated code you need 8 since you emit code for arm target. In this context it should be MONO_ABI_SIZEOF (MonoObject).

Loading a field from a vt type is offset with sizeof (MonoObject) only if the vt is boxed. This looks fishy to me here. Are you sure this code path is being hit and tested properly ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't the type boxed here? The struct T is boxed in SwiftSelf<T>, and we need to load the address of T as the argument.

Copy link
Member

Choose a reason for hiding this comment

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

So SwiftSelf<T> is a struct containing a single field of type T. Therefore the T field is stored at offset 0 in the struct. This struct is passed around by value in C# code, without any boxing happening. Boxing means that the vt is converted to an object and its value copied to that object. Unlike normal valuetypes, objects on the heap contain a sync block, which is this _MonoObject header. If you want to access the value of the T field in a boxed instance, then you would need to offset by the size of this header. I don't see swift interoping dealing with any boxing.

If I'm correct and this code is wrong then this also raises the question on why no tests are failing. Maybe this path is not properly tested and it needs further attention ?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. The MONO_EMIT_NEW_LOAD_MEMBASE_OP was used with non-load instructions. I verified this change locally and confirmed that this code path is being executed. It works with an offset of 0.

}

*sp++ = struct_base_address;

++new_param_count;
Expand Down
3 changes: 1 addition & 2 deletions src/mono/mono/mini/mini-amd64.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,8 +1023,7 @@ get_call_info (MonoMemPool *mp, MonoMethodSignature *sig)

if ((klass == swift_self || klass == swift_indirect_result) && sig->pinvoke) {
guint32 size = mini_type_stack_size_full (m_class_get_byval_arg (klass), NULL, sig->pinvoke && !sig->marshalling_disabled);
g_assert (size == 8);

g_assert (size == SIZEOF_VOID_P);
kotlarmilos marked this conversation as resolved.
Show resolved Hide resolved
ainfo->storage = ArgValuetypeInReg;
ainfo->pair_storage [0] = ArgInIReg;
ainfo->pair_storage [1] = ArgNone;
Expand Down
3 changes: 1 addition & 2 deletions src/mono/mono/mini/mini-arm64.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1869,8 +1869,7 @@ get_call_info (MonoMemPool *mp, MonoMethodSignature *sig)
guint32 align;
MonoType *ptype = mini_get_underlying_type (sig->params [pindex]);
int size = mini_type_stack_size_full (ptype, &align, cinfo->pinvoke);
g_assert (size == 8);

g_assert (size == SIZEOF_VOID_P);
kotlarmilos marked this conversation as resolved.
Show resolved Hide resolved
ainfo->storage = ArgVtypeInIRegs;
ainfo->reg = (klass == swift_self) ? ARMREG_R20 : ARMREG_R8;
ainfo->nregs = 1;
Expand Down
2 changes: 0 additions & 2 deletions src/tests/Interop/Swift/SwiftSelfContext/SwiftSelfContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,13 @@ public struct FrozenNonEnregisteredStruct
public static extern long SumFrozenNonEnregisteredStruct(SwiftSelf<FrozenNonEnregisteredStruct> self);

[Fact]
[SkipOnMono("SwiftSelf<T> is not supported on Mono")]
public unsafe static void TestSelfIsFrozenEnregisteredStruct()
{
long sum = SumFrozenEnregisteredStruct(new SwiftSelf<FrozenEnregisteredStruct>(new FrozenEnregisteredStruct { A = 10, B = 20 }));
Assert.Equal(30, sum);
}

[Fact]
[SkipOnMono("SwiftSelf<T> is not supported on Mono")]
public unsafe static void TestSelfIsFrozenNonEnregisteredStruct()
{
long sum = SumFrozenNonEnregisteredStruct(new SwiftSelf<FrozenNonEnregisteredStruct>(new FrozenNonEnregisteredStruct { A = 10, B = 20, C = 30, D = 40, E = 50 }));
Expand Down
Loading