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

[mono][swift-interop] Support for Swift struct lowering in callback returns #104949

Merged
merged 9 commits into from
Jul 18, 2024
Merged

[mono][swift-interop] Support for Swift struct lowering in callback returns #104949

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a080c25
Enable full SwiftCallbackAbiStress tests
matouskozak Jul 15, 2024
3da512e
[arm64] Swift struct lowering in callback returns
matouskozak Jul 15, 2024
10ce8f1
[amd64] Swift struct lowering in callback returns
matouskozak Jul 15, 2024
0390f16
[x64] refactor return buffer save condition
matouskozak Jul 16, 2024
6e0bbce
fix missing break in epilog
matouskozak Jul 16, 2024
f12e8d2
replace NOT_IMPLEMENTED with not_reached assert
matouskozak Jul 16, 2024
c67df1c
arg_set_val switch case indent
matouskozak Jul 16, 2024
9f0fa6c
refactor LLVM call info comments
matouskozak Jul 17, 2024
540f888
Apply suggestions from code review (by Jeremi)
matouskozak Jul 18, 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
119 changes: 98 additions & 21 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 @@ -988,7 +988,8 @@ get_call_info (MonoMemPool *mp, MonoMethodSignature *sig)
/*
* We need to set this even when sig->pinvoke is FALSE, because the `cinfo` gets copied to the
* `cfg->arch` on the first pass. However, later in `amd64_handle_swift_return_buffer_reg` we
* condition the Swift return buffer handling only to P/Invoke calls.
* condition the Swift return buffer handling only to P/Invoke calls. This however can trigger
* a false positive in some scenarios where the Swift return buffer is not needed.
Copy link
Member

Choose a reason for hiding this comment

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

What are those false positive triggering scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly cases where struct is large enough that it would normally (outside swiftcall) be returned by a reference but small enough that during swiftcall it is returned via registers.

*/
cinfo->need_swift_return_buffer = TRUE;
}
Expand Down Expand Up @@ -1286,17 +1287,17 @@ arg_get_val (CallContext *ccontext, ArgInfo *ainfo, gpointer dest)
int storage_type = ainfo->pair_storage [k];
int reg_storage = ainfo->pair_regs [k];
switch (storage_type) {
case ArgInIReg:
*(gsize*)(storage + ainfo->offsets [k]) = ccontext->gregs [reg_storage];
break;
case ArgInFloatSSEReg:
*(float*)(storage + ainfo->offsets [k]) = *(float*)&ccontext->fregs [reg_storage];
break;
case ArgInDoubleSSEReg:
*(double*)(storage + ainfo->offsets [k]) = ccontext->fregs [reg_storage];
break;
default:
g_assert_not_reached ();
case ArgInIReg:
*(gsize*)(storage + ainfo->offsets [k]) = ccontext->gregs [reg_storage];
break;
case ArgInFloatSSEReg:
*(float*)(storage + ainfo->offsets [k]) = *(float*)&ccontext->fregs [reg_storage];
break;
case ArgInDoubleSSEReg:
*(double*)(storage + ainfo->offsets [k]) = ccontext->fregs [reg_storage];
break;
default:
g_assert_not_reached ();
}
}
break;
Expand All @@ -1314,11 +1315,13 @@ arg_set_val (CallContext *ccontext, ArgInfo *ainfo, gpointer src)
{
g_assert (arg_need_temp (ainfo));

host_mgreg_t *src_cast = (host_mgreg_t*)src;
for (int k = 0; k < ainfo->nregs; k++) {
int storage_type = ainfo->pair_storage [k];
int reg_storage = ainfo->pair_regs [k];
switch (storage_type) {
switch (ainfo->storage) {
case ArgValuetypeInReg: {
host_mgreg_t *src_cast = (host_mgreg_t*)src;
for (int k = 0; k < ainfo->nregs; k++) {
int storage_type = ainfo->pair_storage [k];
int reg_storage = ainfo->pair_regs [k];
switch (storage_type) {
case ArgInIReg:
ccontext->gregs [reg_storage] = *src_cast;
break;
Expand All @@ -1328,9 +1331,38 @@ arg_set_val (CallContext *ccontext, ArgInfo *ainfo, gpointer src)
break;
default:
g_assert_not_reached ();
}
src_cast++;
}
src_cast++;
break;
}
#ifdef MONO_ARCH_HAVE_SWIFTCALL
case ArgSwiftValuetypeLoweredRet: {
char *storage = (char*)src;
for (int k = 0; k < ainfo->nregs; k++) {
int storage_type = ainfo->pair_storage [k];
int reg_storage = ainfo->pair_regs [k];
switch (storage_type) {
case ArgInIReg:
ccontext->gregs [reg_storage] = *(gsize*)(storage + ainfo->offsets [k]);
break;
case ArgInFloatSSEReg:
*(float*)&ccontext->fregs [reg_storage] = *(float*)(storage + ainfo->offsets [k]);
break;
case ArgInDoubleSSEReg:
ccontext->fregs [reg_storage] = *(double*)(storage + ainfo->offsets [k]);
break;
default:
g_assert_not_reached ();
}
}
break;
}
#endif /* MONO_ARCH_HAVE_SWIFTCALL */
default:
g_assert_not_reached ();
}

}

gpointer
Expand Down Expand Up @@ -2368,6 +2400,9 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
linfo->ret.storage = LLVMArgVtypeRetAddr;
linfo->vret_arg_index = cinfo->vret_arg_index;
break;
case ArgSwiftValuetypeLoweredRet:
// LLVM compilation of P/Invoke wrappers is not supported
lambdageek marked this conversation as resolved.
Show resolved Hide resolved
break;
default:
g_assert_not_reached ();
break;
Expand Down Expand Up @@ -2434,6 +2469,9 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
case ArgValuetypeAddrOnStack:
linfo->args [i].storage = LLVMArgVtypeAddr;
break;
case ArgSwiftError:
// LLVM compilation of P/Invoke wrappers is not supported
break;
default:
cfg->exception_message = g_strdup ("ainfo->storage");
cfg->disable_llvm = TRUE;
Expand Down Expand Up @@ -8393,8 +8431,19 @@ MONO_RESTORE_WARNING

if (sig->ret->type != MONO_TYPE_VOID) {
/* Save volatile arguments to the stack */
if (cfg->vret_addr && (cfg->vret_addr->opcode != OP_REGVAR))
amd64_mov_membase_reg (code, cfg->vret_addr->inst_basereg, cfg->vret_addr->inst_offset, cinfo->ret.reg, 8);
if (cfg->vret_addr && (cfg->vret_addr->opcode != OP_REGVAR)) {
#ifdef MONO_ARCH_HAVE_SWIFTCALL
if (mono_method_signature_has_ext_callconv (sig, MONO_EXT_CALLCONV_SWIFTCALL) && sig->pinvoke &&
cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED &&
cfg->arch.cinfo->need_swift_return_buffer && cinfo->ret.reg == AMD64_R10) {
// Save the return buffer passed by the Swift caller
amd64_mov_membase_reg (code, cfg->vret_addr->inst_basereg, cfg->vret_addr->inst_offset, SWIFT_RETURN_BUFFER_REG, 8);
} else
#endif
{
amd64_mov_membase_reg (code, cfg->vret_addr->inst_basereg, cfg->vret_addr->inst_offset, cinfo->ret.reg, 8);
}
}
}

#ifdef MONO_ARCH_HAVE_SWIFTCALL
Expand Down Expand Up @@ -8680,7 +8729,8 @@ mono_arch_emit_epilog (MonoCompile *cfg)

/* Load returned vtypes into registers if needed */
cinfo = cfg->arch.cinfo;
if (cinfo->ret.storage == ArgValuetypeInReg) {
switch (cinfo->ret.storage) {
case ArgValuetypeInReg: {
ArgInfo *ainfo = &cinfo->ret;
MonoInst *inst = cfg->ret;

Expand All @@ -8701,6 +8751,33 @@ mono_arch_emit_epilog (MonoCompile *cfg)
g_assert_not_reached ();
}
}
break;
}
#ifdef MONO_ARCH_HAVE_SWIFTCALL
case ArgSwiftValuetypeLoweredRet: {
if (cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED) {
ArgInfo *ainfo = &cinfo->ret;
MonoInst *ins = cfg->ret;

for (int i = 0; i < ainfo->nregs; i ++) {
matouskozak marked this conversation as resolved.
Show resolved Hide resolved
switch (ainfo->pair_storage [i]) {
case ArgInIReg:
amd64_mov_reg_membase (code, ainfo->pair_regs [i], ins->inst_basereg, ins->inst_offset + ainfo->offsets [i], sizeof (target_mgreg_t));
break;
case ArgInFloatSSEReg:
amd64_movss_reg_membase (code, ainfo->pair_regs [i], ins->inst_basereg, ins->inst_offset + ainfo->offsets [i]);
break;
case ArgInDoubleSSEReg:
amd64_movsd_reg_membase (code, ainfo->pair_regs [i], ins->inst_basereg, ins->inst_offset + ainfo->offsets [i]);
break;
default:
g_assert_not_reached ();
}
}
}
break;
}
#endif /* MONO_ARCH_HAVE_SWIFTCALL */
}

if (cfg->arch.omit_fp) {
Expand Down
88 changes: 70 additions & 18 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 @@ -2024,22 +2024,21 @@ arg_get_val (CallContext *ccontext, ArgInfo *ainfo, gpointer dest)
}
#ifdef MONO_ARCH_HAVE_SWIFTCALL
case ArgSwiftVtypeLoweredRet: {
int i;
int gr = 0, fr = 0; // We can start from 0 since we are handling only returns
char *storage = (char*)dest;
for (i = 0; i < ainfo->nregs; ++i) {
switch (ainfo->struct_storage [i]) {
case ArgInIReg:
*(gsize*)(storage + ainfo->offsets [i]) = ccontext->gregs [gr++];
break;
case ArgInFReg:
*(double*)(storage + ainfo->offsets [i]) = ccontext->fregs [fr++];
break;
case ArgInFRegR4:
*(float*)(storage + ainfo->offsets [i]) = *(float*)&ccontext->fregs [fr++];
break;
default:
g_assert_not_reached ();
for (int k = 0; k < ainfo->nregs; ++k) {
switch (ainfo->struct_storage [k]) {
case ArgInIReg:
*(gsize*)(storage + ainfo->offsets [k]) = ccontext->gregs [gr++];
break;
case ArgInFReg:
*(double*)(storage + ainfo->offsets [k]) = ccontext->fregs [fr++];
break;
case ArgInFRegR4:
*(float*)(storage + ainfo->offsets [k]) = *(float*)&ccontext->fregs [fr++];
break;
default:
g_assert_not_reached ();
}
}
break;
Expand All @@ -2055,10 +2054,39 @@ arg_set_val (CallContext *ccontext, ArgInfo *ainfo, gpointer src)
{
g_assert (arg_need_temp (ainfo));

float *src_float = (float*)src;
for (int k = 0; k < ainfo->nregs; k++) {
*(float*)&ccontext->fregs [ainfo->reg + k] = *src_float;
src_float++;
switch (ainfo->storage) {
case ArgHFA: {
float *src_float = (float*)src;
for (int k = 0; k < ainfo->nregs; k++) {
*(float*)&ccontext->fregs [ainfo->reg + k] = *src_float;
src_float++;
}
break;
}
#ifdef MONO_ARCH_HAVE_SWIFTCALL
case ArgSwiftVtypeLoweredRet: {
int gr = 0, fr = 0; // We can start from 0 since we are handling only returns
char *storage = (char*)src;
for (int k = 0; k< ainfo->nregs; k++) {
matouskozak marked this conversation as resolved.
Show resolved Hide resolved
switch (ainfo->struct_storage [k]) {
case ArgInIReg:
ccontext->gregs [gr++] = *(gsize*)(storage + ainfo->offsets [k]);
break;
case ArgInFReg:
ccontext->fregs [fr++] = *(double*)(storage + ainfo->offsets [k]);
break;
case ArgInFRegR4:
*(float*)&ccontext->fregs [fr++] = *(float*)(storage + ainfo->offsets [k]);
break;
default:
g_assert_not_reached ();
}
}
break;
}
#endif /* MONO_ARCH_HAVE_SWIFTCALL */
default:
g_assert_not_reached ();
}
}

Expand Down Expand Up @@ -6486,6 +6514,30 @@ mono_arch_emit_epilog (MonoCompile *cfg)
}
break;
}
#ifdef MONO_ARCH_HAVE_SWIFTCALL
case ArgSwiftVtypeLoweredRet: {
if (cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED) {
MonoInst *ins = cfg->ret;
int gr = 0, fr = 0; // We can start from 0 since we are handling only returns
for (int i = 0; i < cinfo->ret.nregs; ++i) {
switch (cinfo->ret.struct_storage [i]) {
case ArgInIReg:
code = emit_ldrx (code, gr++, ins->inst_basereg, GTMREG_TO_INT (ins->inst_offset + cinfo->ret.offsets [i]));
break;
case ArgInFRegR4:
code = emit_ldrfpw (code, fr++, ins->inst_basereg, GTMREG_TO_INT (ins->inst_offset + cinfo->ret.offsets [i]));
break;
case ArgInFReg:
code = emit_ldrfpx (code, fr++, ins->inst_basereg, GTMREG_TO_INT (ins->inst_offset + cinfo->ret.offsets [i]));
break;
default:
g_assert_not_reached ();
}
}
}
break;
}
#endif /* MONO_ARCH_HAVE_SWIFTCALL */
default:
break;
}
Expand Down
Loading
Loading