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.

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

[mono][swift-interop] Support for Swift struct lowering in direct P/Invoke returns #104389

Merged

Conversation

matouskozak
Copy link
Member

@matouskozak matouskozak commented Jul 3, 2024

Description

This PR adds support in Mono runtime for Swift @frozen struct lowering in returns. The scope is limited to direct P/Invoke scenario with reverse P/Invoke following up in a separate PR. The changes cover support over all Mono supported backend engines: miniJIT, interpreter and AOT.

In Swift, structures can be lowered into <= 4 primitives that are subsequently returned via registers. This is a simplified scenario in comparison to lowering of arguments where lowered elements can also be passed on stack when maximum number of registers used for arguments is exceeded.

Implementation

The lowering support for returns is handled at the codegen level (mini-<arch>.c) by introducing new storage type (ArgSwiftVtypeLoweredRet/ArgSwiftValuetypeLoweredRet) that represents a structure that is returned via combination of integer and float registers.

The first step of struct lowering is handled in get_call_info -> add_return_valuetype_swiftcall where struct lowering is computed. In scenarios where struct can be lowered (fits in <= 4 registers) the newly introduced storage type is used, otherwise the struct is returned via reference.

  1. Returned by lowered sequence:

    • The re-composition of struct is handled in emit_move_return_value where the values stored in return registers are loaded into prepared memory at specific offsets.
  2. Returned by reference:

    • On arm64, handled as regular ArgVtypeByRef with no extra changes.
    • On x64, handled as ArgValuetypeAddrInIReg with specific handling where the reference is temporarily hold in R10 reg. and moved to RAX reg. (amd64_handle_swift_return_buffer_reg) before emitting the call to native code. That's because Swift expects that the return buffer reference is passed in RAX reg. but in Mono runtime, this reg. serves multiple purposes and we cannot easily prevent RAX clobbering by other parts of code.

Verification

The changes are verified using the Interop/Swift/SwiftRetAbiStress runtime tests. The extended set of 5000 test cases was used locally to further verify the lowering support. The fullAOT functionality was only verified in local settings due to missing coverage on CI.

Other changes

  • refactoring of argument Swift @frozen struct lowering in method-to-ir.c
  • refactoring of Swift return buffer handling (amd64_handle_swift_return_buffer_reg)
  • adding more #ifdef MONO_ARCH_HAVE_SWIFTCALL to better separate swiftcall support
  • add missing ArgSwiftError entry in mono_arch_get_llvm_call_info on arm64. This is just a placeholder because we don't compile P/Invoke wrappers by LLVM. However, on arm64, the missing entry was causing failures in AOT-llvm compilation.

Contributes to #102077

@matouskozak matouskozak added area-Codegen-meta-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) arch-arm64 arch-x64 and removed area-Codegen-JIT-mono labels Jul 3, 2024
Copy link
Contributor

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

@matouskozak matouskozak added this to the 9.0.0 milestone Jul 3, 2024
@matouskozak matouskozak changed the title [WIP][mono][swift-interop] Support for Swift struct lowering in direct P/Invoke returns [mono][swift-interop] Support for Swift struct lowering in direct P/Invoke returns Jul 4, 2024
@matouskozak matouskozak marked this pull request as ready for review July 4, 2024 11:12
@matouskozak matouskozak removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 4, 2024
@matouskozak matouskozak requested a review from BrzVlad July 9, 2024 07:12
Copy link
Member

@jkurdek jkurdek left a comment

Choose a reason for hiding this comment

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

Good job!

You mentioned you tested these changes on extended set of 5000 tests in Interop/Swift/SwiftRetAbiStress. Is there any description somewhere how to get / generate this set? Could be useful for testing other changes.

*sp++ = struct_base_address;
ptype = mono_class_get_byref_type (mono_defaults.typed_reference_class);
Copy link
Member

Choose a reason for hiding this comment

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

It is not clear to me why we can use mono_class_get_byref_type (mono_defaults.typed_reference_class) instead of 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.

It is not clear to me why we can use mono_class_get_byref_type (mono_defaults.typed_reference_class) instead of mono_class_get_byref_type (klass);

I'm not sure either. I did some investigating and it seems that the all byref types get changed to generic MONO_TYPE_I which is then handled by codegen as a pointer-sized integer. I will change it to mono_class_get_byref_type (klass) as it makes more sense.

@@ -1179,7 +1179,7 @@ mono_arch_get_interp_to_native_trampoline (MonoTrampInfo **info)

/* save all return floating registers in the CallContext */
for (i = 0; i < FLOAT_RETURN_REGS; i++)
amd64_sse_movsd_membase_reg (code, AMD64_R11, MONO_STRUCT_OFFSET (CallContext, fregs) + i * sizeof (double), i);
amd64_sse_movsd_membase_reg (code, AMD64_R11, MONO_STRUCT_OFFSET (CallContext, fregs) + float_return_regs [i] * sizeof (double), float_return_regs [i]);
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 this change necessary only for amd64?

Copy link
Member Author

Choose a reason for hiding this comment

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

On arm64, the argument and return registers are the same (x0-x7). Whereas on amd64, different registers are used for arguments and returns.

For arm64, this gets handled at

/* set all general purpose registers to CallContext */
for (i = 0; i < PARAM_REGS; i++)
arm_strx (code, i, ARMREG_IP0, MONO_STRUCT_OFFSET (CallContext, gregs) + i * sizeof (host_mgreg_t));
/* set all floating registers to CallContext */
for (i = 0; i < FP_PARAM_REGS; i++)
arm_strfpx (code, i, ARMREG_IP0, MONO_STRUCT_OFFSET (CallContext, fregs) + i * sizeof (double));

src/mono/mono/mini/mini-arm64.c Outdated Show resolved Hide resolved
@fanyang-mono
Copy link
Member

Is there any follow-up work we need to do for LLVM?

@@ -176,6 +176,10 @@ struct sigcontext {
* reproduceable results for benchmarks */
#define MONO_ARCH_CODE_ALIGNMENT 32

#if defined(TARGET_OSX) || defined(TARGET_APPLE_MOBILE)
#define MONO_ARCH_HAVE_SWIFTCALL 1
Copy link
Member

Choose a reason for hiding this comment

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

Is there a better place, where we could set this value for Mono instead of repeating it for each architecture? Like somewhere in cmake logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it could be set in src/mono/CMakeLists.txt by something like:

if ((TARGET_AMD64 OR TARGET_ARM64) AND (TARGET_OSX or TARGET_APPLE_MOBILE))
    set(MONO_ARCH_HAVE_SWIFTCALL 1)
endif()

However, CoreCLR also does set it in the respective arch files e.g.

#define SWIFT_SUPPORT

What do you think is better? (cc: @kotlarmilos, @jkurdek)

Copy link
Member

Choose a reason for hiding this comment

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

It might be cleaner to move it to src/mono/CMakeLists.txt, but don't have a strong opinion about it. Since this is not a new change, I suggest doing it in a follow-up PR.

@matouskozak
Copy link
Member Author

Is there any follow-up work we need to do for LLVM?

At the moment no, Mono doesn't use LLVM to compile P/Invoke wrappers so it fallbacks to Mini. I just added the entries inside mono_arch_get_llvm_call_info for arm64 to avoid asserting there (not needed on amd64)

@kotlarmilos kotlarmilos self-requested a review July 12, 2024 14:31
@matouskozak matouskozak merged commit b885a58 into dotnet:main Jul 15, 2024
122 of 126 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2024
@matouskozak matouskozak deleted the swift-interop/pinvoke-struct-returns branch October 3, 2024 13:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants