Skip to content

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

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

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 all 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
12 changes: 10 additions & 2 deletions 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

@jakobbotsch jakobbotsch Oct 4, 2024

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
Contributor

@jkurdek jkurdek Oct 4, 2024

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

@kotlarmilos kotlarmilos Oct 4, 2024

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

@jakobbotsch jakobbotsch Oct 4, 2024

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 @@ -3436,8 +3439,13 @@ interp_emit_swiftcall_struct_lowering (TransformData *td, MonoMethodSignature *c
g_array_append_val (new_params, lowered_swift_struct.lowered_elements [idx_lowered]);
}
} 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);
// For structs that cannot be lowered, we change the argument to a pointer-like argument type.
// If SwiftSelf<T> can't be lowered, it should be passed in the same manner as SwiftSelf, via the context register.
if (gklass && (gklass->container_class == swift_self_t))
ptype = mono_class_get_byref_type (swift_self);
else
ptype = mono_class_get_byref_type (klass);

// 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
14 changes: 12 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 @@ -7562,9 +7564,17 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
++new_param_count;
}
} else {
// For structs that cannot be lowered, we change the argument to byref type
// For structs that cannot be lowered, we change the argument to a pointer-like argument type.
// If SwiftSelf<T> can't be lowered, it should be passed in the same manner as SwiftSelf, via the context register.
if (gklass && (gklass->container_class == swift_self_t)) {
ptype = mono_class_get_byref_type (swift_self);
// The ARGLOADA should be a pointer-like type.
struct_base_address->klass = mono_defaults.int_class;
Copy link
Member

@BrzVlad BrzVlad Jul 17, 2024

Choose a reason for hiding this comment

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

I don't think this is needed. Is it leading to failures, maybe it was just failing with previous wrong code ? It seems to me in other places the klass information holds the type that we get the ref for and not really the ref type.

} else {
ptype = mono_class_get_byref_type (klass);
}

*sp++ = struct_base_address;
ptype = mono_class_get_byref_type (klass);

g_array_append_val (new_params, ptype);
++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 @@ -1095,8 +1095,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 == TARGET_SIZEOF_VOID_P);
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 @@ -1922,8 +1922,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 == TARGET_SIZEOF_VOID_P);
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