[mono][swift-interop] Support for Swift frozen struct lowering in mini-JIT/AOT/interpreter #102143

matouskozak · 2024-05-13T05:25:53Z

Description

This PR adds support for lowering of Swift @frozen structs in P/Invoke arguments. In short, in Swift calls, @frozen structs can be passed as a sequence of up to 4 primitives using registers or stack (see Swift Calling Convention). The general algorithm was implemented in #99439 and this PR connects it to the Mono codegen backends. This PR doesn't handle returns of Swift @frozen structs and reverse P/Invoke scenarios (tracked at #102077).

Changes

The connection to codegen is done at Mono IR level. First, in method-to-ir.c for connection to JIT and AOT backends. Second, in transform.c for interpreter. Both connection are similar.

Before doing call to native Swift code, we explore the signature for any struct types. If struct encountered (not SwiftSelf or SwiftError which are handled individually), we execute the lowering algorithm. Based on the result we have 2 outcomes:

Structure can be lowered into a sequence of up to 4 primitives:
The structs argument in signature is replaced by the lowered sequence. The sequence elements are taken from the original structs at previously calculated offsets (struct base address + element offset). For detail how are the offsets calculated see mono_marshal_get_swift_physical_lowering.
Structure must be passed by reference:
The structs argument in signature is replaced by a byref type that represents address of the structs.

Non-struct types are preserved. The modified signature is used just for the call to native Swift code (for non-P/Invoke calls original signature is used).

Lastly, refactoring and bug fixes for marshaling inside mono_marshal_get_swift_physical_lowering.

changing returned lowering from MonoTypeEnum to MonoType* as this is the most common representation of types in the later Mono code flow
fixing and refactoring algorithm for merging opaque intervals (previous version wasn't respecting correctly block boundaries)
use correct field offset and value type size (taking into account MONO_ABI_SIZEOF (MonoObject))
fix remaining_interval_size for final lowering loop

Verification

Currently, Mono CI only has coverage for JIT/interpreter but fullAOT verification was done locally. This PR passes an extended SwiftAbiStressTest of 5000 signature locally both on x64 and arm64 with JIT/interpreter/fullAOT backends.

Alternatives

During preparation of this PR, several alternatives were explore and decided not to further proceed.

Implementing the lowering at the lower codegen level (mini-<arch>.c files). This option was discarded because Mini currently doesn't provide representation for value types that can be passed via registers and stack simultaneously. While handling it manually for this PR would be possibly, the final code was long and error prone. Additionally, the code would have to be duplicated for each architecture.
Implementing the lowering at IL-gen level (marshal-lightweight.c). This option was successfully implemented and provided a shared implementation for both Mini and interpreter. However, we decided to rather implement it at the Mono IR level. This has two benefits, firstly we get better control over the resulting assembly as we are closer to the Mono codegen. Secondly, we are able to keep interpreter code more independent for potential deployment of interpreter outside Mono.

FYI: @jakobbotsch @amanasifkhalid @jkurdek @vitek-karas

dotnet-policy-service · 2024-05-13T05:27:45Z

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

outside Swift struct lowering algorithm

src/mono/mono/metadata/marshal.c

lambdageek

mostly lgtm. I'm not 100% sure about using memcpy on StackInfo in the interpreter.

I think (for the JIT especially) we need to be careful about where we allocate the new signature.

It would be nice if we could avoid g_array_new - can we have a faster path for, let's say, upto 16 arguments that uses a temporary stack allocation for the working array?

I think it might make sense to move the code for doing the lowering into a helper function in transform.c (and in method-to-ir.c) rather than doing everything inline in the giant switch statements. That might keep the code a bit cleaner and easier to skip when someone is reading and they're not interested in Swift

src/mono/mono/metadata/marshal.c

lambdageek · 2024-05-14T14:23:57Z

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

+		StackInfo *sp_old_params = (StackInfo*) mono_mempool_alloc (td->mempool, sizeof (StackInfo) * csignature->param_count);
+		memcpy (sp_old_params, td->sp, sizeof (StackInfo) * csignature->param_count);
+
+		GArray *new_params = g_array_new (FALSE, FALSE, sizeof (MonoType*));


Can you use g_array_sized_new with an initial size equal to the old param count (plus 4?). that would avoid resizing most of the time.

Yes, I changed it to use the old param count since we are unsure ahead-of-time if any lowering will occur.

lambdageek · 2024-05-14T14:28:49Z

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

+				// Create a new dummy signature with the lowered arguments
+				MonoMethodSignature *swiftcall_signature = mono_metadata_signature_alloc (m_class_get_image (method->klass), new_param_count);
+				for (uint32_t idx_param = 0; idx_param < new_param_count; ++idx_param) {
+					swiftcall_signature->params [idx_param] = g_array_index (new_params, MonoType *, idx_param);
+				}
+				swiftcall_signature->ret = fsig->ret;
+				swiftcall_signature->pinvoke = fsig->pinvoke;
+				swiftcall_signature->ext_callconv = fsig->ext_callconv;
+
+				fsig = swiftcall_signature;	


Can we make a helper function and share it between the JIT and interp?

I'm not sure m_class_get_image (method->klass) is where we want to allocate the signature. If the method belongs to a generic instance, we should use the mem manager of the generic instance (basically a mem manager that depends on the assemblies of the generic definition and the generic type params).

Good point, I created a new helper function mono_metadata_signature_dup_new_params insidemetadata.c.

kotlarmilos

Thanks for making changes verbose.

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

The helper function duplicates signature metadata and adds new parameters to the signature. + pre-allocating new_params to the old params size

Only for interpreter at the moment because mono_method_to_ir is more complicated due to NEW_ARGLOADA macros

matouskozak · 2024-05-15T17:11:29Z

Thank you for the feedback. I made several changes to improve the PR.

mostly lgtm. I'm not 100% sure about using memcpy on StackInfo in the interpreter.

Done, removed the use of memcpy on StackInfo.

I think (for the JIT especially) we need to be careful about where we allocate the new signature.

Done, changed the allocation of new signature to use cfg->mempool and td->mempool for mini and interpreter, respectively.

It would be nice if we could avoid g_array_new - can we have a faster path for, let's say, upto 16 arguments that uses a temporary stack allocation for the working array?

We discussed that offline and agreed to stick with current dynamic allocation because we don't know the final number of parameters up front. In the future, we might change the process and collect the struct lowering information a little bit earlier (e.g. during marshalling

runtime/src/mono/mono/metadata/marshal.c

Lines 3698 to 3730 in 19467dc

    
           if (mono_method_signature_has_ext_callconv (csig, MONO_EXT_CALLCONV_SWIFTCALL)) { 
        
           	MonoClass *swift_self = mono_class_try_get_swift_self_class (); 
        
           	MonoClass *swift_error = mono_class_try_get_swift_error_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; 
        
           	for (int i = 0; i < method->signature->param_count; ++i) { 
        
           		MonoClass *param_klass = mono_class_from_mono_type_internal (method->signature->params [i]); 
        
           		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) { 
        
           				swift_self_args++; 
        
           			} else if (!m_class_is_blittable (param_klass) || m_class_is_simd_type (param_klass)) { 
        
           				swift_error_args = swift_self_args = 0; 
        
           				mono_error_set_generic_error (emitted_error, "System", "InvalidProgramException", "Passing non-blittable types to a P/Invoke with the Swift calling convention is unsupported."); 
        
           				break; 
        
           			} 
        
           		} 
        
           	} 
        
           	if (swift_self_args > 1 || swift_error_args > 1) { 
        
           		mono_error_set_generic_error (emitted_error, "System", "InvalidProgramException", "Method signature contains multiple SwiftSelf or SwiftError arguments."); 
        
           	} 
        
           	if (!is_ok (emitted_error)) { 
        
           		get_marshal_cb ()->mb_emit_exception_for_error (mb, emitted_error); 
        
           		goto emit_exception_for_error; 
        
           	} 
        
           }

) and save it in a temporary "swift-interop" mempool. Consequently, we would have more information for the lower layers and that could save up some allocations and speed up the code.

I think it might make sense to move the code for doing the lowering into a helper function in transform.c (and in method-to-ir.c) rather than doing everything inline in the giant switch statements. That might keep the code a bit cleaner and easier to skip when someone is reading and they're not interested in Swift

Done for interpreter by creating interp_emit_swiftcall_struct_lowering helper function. I tried it for mini in method-to-ir.c as well but run into issues with increasing the shared stack pointer (sp) over the original amount of arguments.

jkoritzinsky

Changes to the lowering algorithm LGTM

kotlarmilos

LGTM!

BrzVlad · 2024-05-17T08:19:52Z

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

+{
+	MonoMethodSignature *new_csignature;
+	g_assert (!csignature->hasthis); // P/Invoke calls shouldn't contain 'this'
+


The argument moving around that happens here will break the fast offset allocator (where we allocate storage for vars immediately as we push it on the td->sp stack). This appears however to be fine because we disable tiering for wrappers and I would imagine a swift call is only happening from a m2n wrapper. However I would assert here that td->optimized with the comment that argument reordering here doesn't handle on the fly offset allocation and requires the full var offset allocator pass that is only ran for optimized code.

Yes, we only need to perform the struct lowering for m2n (and later eventually n2m) wrappers. I added the assert with the comment you suggested.

matouskozak · 2024-05-17T12:17:24Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-05-17T12:17:41Z

Azure Pipelines successfully started running 1 pipeline(s).

…i-JIT/AOT/interpreter (dotnet#102143) * refactor + bug fixes for swift struct marshalling * mini struct lowering * interpreter swift struct lowering * enable SwiftAbiStress on Mono

matouskozak added 4 commits May 10, 2024 10:39

refactor + bug fixes for swift struct marshalling

d664b69

mini struct lowering

b305cb3

interpreter swift struct lowering

2764076

enable SwiftAbiStress on Mono

5294446

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 13, 2024

dotnet-policy-service bot assigned matouskozak May 13, 2024

matouskozak changed the title ~~[mono] [swift-interop Swift interop/pinvoke struct lowering~~ [mono][swift-interop] Support for Swift frozen struct lowering in mini-JIT/AOT/interpreter May 13, 2024

This was referenced May 13, 2024

Crash in Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.WorkItemExecution #90019

Open

tracing/eventpipe/providervalidation/providervalidation/providervalidation.cmd test failure #101759

Closed

explicit handling of byref swift structs

91a5e7a

kotlarmilos mentioned this pull request May 14, 2024

Implement support for Swift frozen structs in Mono runtime #102077

Closed

6 tasks

move SwiftSelf and SwiftError checks

49d5afb

outside Swift struct lowering algorithm

matouskozak commented May 14, 2024

View reviewed changes

src/mono/mono/metadata/marshal.c Show resolved Hide resolved

matouskozak removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels May 14, 2024

matouskozak marked this pull request as ready for review May 14, 2024 14:09

matouskozak requested review from BrzVlad, kotlarmilos and lambdageek as code owners May 14, 2024 14:09

matouskozak requested review from steveisok and thaystg as code owners May 14, 2024 14:09

matouskozak requested a review from jkoritzinsky May 14, 2024 14:09

lambdageek reviewed May 14, 2024

View reviewed changes

kotlarmilos reviewed May 14, 2024

View reviewed changes

src/mono/mono/mini/method-to-ir.c Outdated Show resolved Hide resolved

src/mono/mono/mini/method-to-ir.c Outdated Show resolved Hide resolved

src/mono/mono/mini/method-to-ir.c Outdated Show resolved Hide resolved

src/mono/mono/mini/method-to-ir.c Show resolved Hide resolved

matouskozak added 2 commits May 14, 2024 19:47

Adding a helper function for new signature

b564238

The helper function duplicates signature metadata and adds new parameters to the signature. + pre-allocating new_params to the old params size

extract swift struct lowering to separate function

96a6fd3

Only for interpreter at the moment because mono_method_to_ir is more complicated due to NEW_ARGLOADA macros

build-analysis bot mentioned this pull request May 15, 2024

NativeAOT legs timing out in CI #102239

Closed

matouskozak added 3 commits May 15, 2024 15:02

fix build warnings in signature alloc helper func

ba829c8

deallocate temporary arrays

241765b

fix cast to guint16 in transform.c

0638729

jkoritzinsky approved these changes May 15, 2024

View reviewed changes

build-analysis bot mentioned this pull request May 15, 2024

[browser][MT] mono_download_assets - RangeError: Start offset -1 is outside the bounds of the buffer #102196

Closed

kotlarmilos self-requested a review May 16, 2024 08:44

kotlarmilos approved these changes May 16, 2024

View reviewed changes

BrzVlad reviewed May 17, 2024

View reviewed changes

matouskozak added 2 commits May 17, 2024 10:32

fix style in marshaling of swift structs

2376d05

assert td->optimized for interp struct lowering

f0e8da4

BrzVlad approved these changes May 17, 2024

View reviewed changes

matouskozak merged commit b1b3e85 into dotnet:main May 20, 2024
154 of 160 checks passed

matouskozak mentioned this pull request May 21, 2024

[mono][interp] Update mono struct lowering to use MonoMemoryManager #102486

Merged

github-actions bot locked and limited conversation to collaborators Jun 21, 2024

matouskozak deleted the swift-interop/pinvoke-struct-lowering branch October 3, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono][swift-interop] Support for Swift frozen struct lowering in mini-JIT/AOT/interpreter #102143

[mono][swift-interop] Support for Swift frozen struct lowering in mini-JIT/AOT/interpreter #102143

matouskozak commented May 13, 2024 •

edited

Loading

dotnet-policy-service bot commented May 13, 2024

lambdageek left a comment

lambdageek May 14, 2024

matouskozak May 14, 2024

lambdageek May 14, 2024

matouskozak May 14, 2024

kotlarmilos left a comment

matouskozak commented May 15, 2024

jkoritzinsky left a comment

kotlarmilos left a comment

BrzVlad May 17, 2024

matouskozak May 17, 2024

matouskozak commented May 17, 2024

azure-pipelines bot commented May 17, 2024

[mono][swift-interop] Support for Swift frozen struct lowering in mini-JIT/AOT/interpreter #102143

[mono][swift-interop] Support for Swift frozen struct lowering in mini-JIT/AOT/interpreter #102143

Conversation

matouskozak commented May 13, 2024 • edited Loading

Description

Changes

Verification

Alternatives

dotnet-policy-service bot commented May 13, 2024

lambdageek left a comment

Choose a reason for hiding this comment

lambdageek May 14, 2024

Choose a reason for hiding this comment

matouskozak May 14, 2024

Choose a reason for hiding this comment

lambdageek May 14, 2024

Choose a reason for hiding this comment

matouskozak May 14, 2024

Choose a reason for hiding this comment

kotlarmilos left a comment

Choose a reason for hiding this comment

matouskozak commented May 15, 2024

jkoritzinsky left a comment

Choose a reason for hiding this comment

kotlarmilos left a comment

Choose a reason for hiding this comment

BrzVlad May 17, 2024

Choose a reason for hiding this comment

matouskozak May 17, 2024

Choose a reason for hiding this comment

matouskozak commented May 17, 2024

azure-pipelines bot commented May 17, 2024

matouskozak commented May 13, 2024 •

edited

Loading