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

kotlarmilos · 2024-06-28T18:45:27Z

Description

This PR adds Mono support for SwiftSelf<T> in the Swift calling convention. This type enables the correct passing of frozen value types in instance methods in Swift.

Changes

This PR includes both Mono JIT and Interpreter changes. The lowering has been modified to handle SwiftSelf<T> as any struct if it can be enregistered. Otherwise, the generic value is unboxed and handled as SwiftSelf.

Verification

The runtime tests in Interop/Swift/SwiftSelf should verify the functionality.

Additional notes

Additional testing coverage will be added as we progress with projection tooling changes.

Contributes to #102079

dotnet-policy-service · 2024-06-28T18:45:50Z

Tagging subscribers to this area: @steveisok, @lambdageek
See info in area-owners.md if you want to be subscribed.

kotlarmilos · 2024-06-28T18:46:16Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-06-28T18:48:36Z

Pipelines were unable to run due to time out waiting for the pull request to finish merging.

lambdageek · 2024-06-28T19:30:47Z

src/mono/mono/metadata/marshal.c

 				MonoClass *param_klass = mono_class_from_mono_type_internal (method->signature->params [i]);
+				bool is_swift_self_generic = (strcmp(param_klass->name, "SwiftSelf`1") == 0) && (strcmp(param_klass->name_space, "System.Runtime.InteropServices.Swift") == 0);


This is not the right way to check that param_klass is an instance of a known generic. (you should not rely on the name/name_space of a generic instance being the same as the gtd; also you need to check that the image of the definition is CoreLib).

you want something like:

GENERATE_TRY_GET_CLASS_WITH_CACHE_DECL(swift_self_t); GENERATE_TRY_GET_CLASS_WITH_CACHE(swift_self_t, "System.Runtime.InteropServices.Swift", "SwiftSelf`1"); gboolean is_swift_self_inst (MonoClass *klass) { MonoGenericClass *gklass = mono_class_try_get_generic_class (klass); if (!gklass) return FALSE; return gklass->container_class == mono_class_try_get_swift_self_t_class (); }

Thanks! Please let me know if you prefer a helper method (as you outlined). Since there are already many helper methods, I am wondering if this would improve readability.

lambdageek · 2024-06-28T19:33:17Z

src/mono/mono/mini/interp/transform.c

+				if ((strcmp (klass->name, "SwiftSelf`1") == 0) && (strcmp (klass->name_space, "System.Runtime.InteropServices.Swift") == 0))
+					ptype = mono_class_get_byref_type (swift_self);
+				else
+					ptype = mono_class_get_byref_type (mono_defaults.typed_reference_class);


I know this bit isn't new, but did we add TypedReference as a dynamic dependency to SwiftSelf / SwiftSelf<T> for the linker?

I don't think so. Would the build fail if it's not added as a dynamic dependency?

lambdageek

I'm not sure hardcoding "8" is an improvement in readability

Also I'm not sure I understood what was happening with SwiftSelf/TypedReference

lambdageek · 2024-06-28T19:34:42Z

src/mono/mono/mini/mini-amd64.c

 				ainfo->storage = ArgValuetypeInReg;
 				ainfo->pair_storage [0] = ArgInIReg;
 				ainfo->pair_storage [1] = ArgNone;
 				ainfo->nregs = 1;
 				ainfo->pair_regs [0] = GINT32_TO_UINT8 (AMD64_R13);
-				ainfo->pair_size [0] = size;
+				ainfo->pair_size [0] = 8;


What is this 8? is that the size of a byref?

I know this code is arch specific, but SIZEOF_VOID_P might be better.

Also what about TypedReference it's not size 8 on mono:

https://github.com/dotnet/runtime/blob/main/src/mono/System.Private.CoreLib/src/System/TypedReference.Mono.cs

Initially, I tried to pass SwiftSelf<T> here, instead of SwiftSelf and encountered issues with size. Since it is transformed to SwiftSelf if not enregistered, I've reverted all mini-<arch> changes.

lambdageek · 2024-06-28T19:36:45Z

src/mono/mono/mini/mini-arm64.c

 				ainfo->storage = ArgVtypeInIRegs;
 				ainfo->reg = ARMREG_R20;
 				ainfo->nregs = 1;
-				ainfo->size = size;
+				ainfo->size = 8;


ditto. i'm not sure what 8 means here

lambdageek · 2024-06-28T19:39:29Z

src/mono/mono/mini/interp/transform.c

+				if ((strcmp (klass->name, "SwiftSelf`1") == 0) && (strcmp (klass->name_space, "System.Runtime.InteropServices.Swift") == 0))
+					ptype = mono_class_get_byref_type (swift_self);
+				else
+					ptype = mono_class_get_byref_type (mono_defaults.typed_reference_class);


Actually... what is this? TypedReference& is not ok since it's a byref of a ByRefLike type. we don't really support that.

Do you just need some pointer-sized type in here as a placeholder?

mono_defaults.int_class is usually ok

Yes, we need some pointer-sized type in here as a placeholder. Updated according to your suggestion.

kotlarmilos · 2024-06-29T15:55:21Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-06-29T15:55:33Z

Azure Pipelines successfully started running 1 pipeline(s).

jkurdek · 2024-07-09T12:52:09Z

src/mono/mono/mini/method-to-ir.c

+							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);


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); ?

#104171 (comment)

@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.

src/mono/mono/mini/mini-amd64.c

src/mono/mono/mini/mini-arm64.c

lambdageek

Seems ok to me, although i'm trusting that the Swift ABI is being respected - I'm not an expert on that part

BrzVlad · 2024-07-11T15:08:12Z

src/mono/mono/mini/interp/transform.c

+				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);


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.

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.

BrzVlad · 2024-07-11T15:14:59Z

src/mono/mono/mini/method-to-ir.c

+								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);


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

Yes, you are right. I was missing sizeof (MonoObject).

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...

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 ?

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.

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 ?

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.

Co-authored-by: Aleksey Kliger (λgeek) <[email protected]>

kotlarmilos · 2024-07-16T11:55:28Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-07-16T11:55:41Z

Azure Pipelines successfully started running 1 pipeline(s).

kotlarmilos · 2024-07-16T12:00:51Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-07-16T12:01:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2024-07-16T12:22:47Z

src/mono/mono/mini/method-to-ir.c

+							{
+								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_BIALU_IMM (cfg, OP_ADD_IMM, struct_base_address->dreg, swift_self_base_address->dreg, 0);


After all these discussions, it raises the trivial question. What is the point of this then ? We are just adding 0 to an address :)

The instruction was not necessary and was converting SwiftSelf<T> to IntPtr. Using SwiftSelf<T> in the ARGLOADA was resulting in an assertion in the IR at some point. I've removed this instruction and updated the ARGLOADA instruction to be a pointer-like type. Please let me know if you have any other suggestions.

BrzVlad · 2024-07-17T07:20:13Z

src/mono/mono/mini/method-to-ir.c

+							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;


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.

kotlarmilos · 2024-07-19T09:01:51Z

/ba-g Timeout failures on WASM.

kotlarmilos · 2024-07-19T09:06:42Z

Merging this PR to be feature complete. We will continue investigation on why the ARGLOADA should require a pointer-like type for SwiftSelf<T>.

jakobbotsch · 2024-10-04T11:38:48Z

src/mono/mono/mini/interp/transform.c

@@ -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);


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.

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?

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

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.

kotlarmilos added 3 commits June 28, 2024 20:39

Add support for SwiftSelf<T>

0647f6a

Add constraints

3008c6c

Enable runtime tests

240c1b3

kotlarmilos added area-Codegen-meta-mono os-ios Apple iOS labels Jun 28, 2024

kotlarmilos added this to the 9.0.0 milestone Jun 28, 2024

kotlarmilos requested review from jakobbotsch, matouskozak, amanasifkhalid and jkurdek June 28, 2024 18:45

kotlarmilos self-assigned this Jun 28, 2024

kotlarmilos requested review from BrzVlad, lambdageek, steveisok and thaystg as code owners June 28, 2024 18:45

kotlarmilos mentioned this pull request Jun 28, 2024

Runtime support for Swift Interop in .NET 9 #93631

Closed

42 tasks

lambdageek reviewed Jun 28, 2024

View reviewed changes

kotlarmilos added 2 commits June 29, 2024 17:43

Add swift_self_t instead of strcmp check

5bcb0a5

Use int_class instead of typed_reference

630e6a9

jkurdek reviewed Jul 9, 2024

View reviewed changes

jkurdek approved these changes Jul 9, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jul 9, 2024

[browser] addons.xpi ERROR System addon update list error SyntaxError: XMLHttpRequest.open: 'http://%(server)s/dummy-system-addons.xml' is not a valid URL. #104580

Closed

lambdageek reviewed Jul 10, 2024

View reviewed changes

src/mono/mono/mini/mini-amd64.c Outdated Show resolved Hide resolved

lambdageek reviewed Jul 10, 2024

View reviewed changes

src/mono/mono/mini/mini-arm64.c Outdated Show resolved Hide resolved

lambdageek approved these changes Jul 10, 2024

View reviewed changes

BrzVlad reviewed Jul 11, 2024

View reviewed changes

kotlarmilos and others added 6 commits July 12, 2024 15:57

Update src/mono/mono/mini/mini-amd64.c

692aacc

Co-authored-by: Aleksey Kliger (λgeek) <[email protected]>

Update src/mono/mono/mini/mini-arm64.c

5756236

Co-authored-by: Aleksey Kliger (λgeek) <[email protected]>

Use sizeof(MonoObject) offset for swift_self_t

04eee19

Merge branch 'main' into feature/swift-self-generic

0b05920

Use MONO_ABI_SIZEOF to load boxed struct as an argument

110534a

Load the address of the struct with 0 offset from SwiftSelf<T>

b902f1d

Resolve conflicts

30c14b0

BrzVlad reviewed Jul 16, 2024

View reviewed changes

Update NEW_ARGLOADA to the pointer-like type

21f7fc7

This was referenced Jul 16, 2024

[browser] RuntimeError: memory access out of bounds #96546

Closed

WASM test failing with OOM #100111

Closed

BrzVlad reviewed Jul 17, 2024

View reviewed changes

BrzVlad approved these changes Jul 17, 2024

View reviewed changes

kotlarmilos merged commit 3648b56 into dotnet:main Jul 19, 2024
123 of 126 checks passed

github-actions bot locked and limited conversation to collaborators Aug 19, 2024

jakobbotsch reviewed Oct 4, 2024

View reviewed changes

		MonoClass *param_klass = mono_class_from_mono_type_internal (method->signature->params [i]);
		bool is_swift_self_generic = (strcmp(param_klass->name, "SwiftSelf`1") == 0) && (strcmp(param_klass->name_space, "System.Runtime.InteropServices.Swift") == 0);

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

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

Conversation

kotlarmilos commented Jun 28, 2024

Description

Changes

Verification

Additional notes

dotnet-policy-service bot commented Jun 28, 2024

kotlarmilos commented Jun 28, 2024

azure-pipelines bot commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotlarmilos commented Jun 29, 2024

azure-pipelines bot commented Jun 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrzVlad Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotlarmilos commented Jul 16, 2024

azure-pipelines bot commented Jul 16, 2024

kotlarmilos commented Jul 16, 2024

azure-pipelines bot commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotlarmilos commented Jul 19, 2024

kotlarmilos commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

lambdageek Jun 28, 2024 •

edited

Loading

BrzVlad Jul 11, 2024 •

edited

Loading