Skip to content

JIT: Support loading Swift self register from SwiftSelf struct #99132

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
amanasifkhalid merged 4 commits into from
Mar 1, 2024
Merged

JIT: Support loading Swift self register from SwiftSelf struct #99132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fabf679
Enable test locally
amanasifkhalid Feb 29, 2024
843333f
Set SwiftSelf register
amanasifkhalid Feb 29, 2024
3cfebf0
Style
amanasifkhalid Feb 29, 2024
434575d
Revert test changes
amanasifkhalid Feb 29, 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
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4367,7 +4367,7 @@ class Compiler
void impCheckForPInvokeCall(
GenTreeCall* call, CORINFO_METHOD_HANDLE methHnd, CORINFO_SIG_INFO* sig, unsigned mflags, BasicBlock* block);
GenTreeCall* impImportIndirectCall(CORINFO_SIG_INFO* sig, const DebugInfo& di = DebugInfo());
void impPopArgsForUnmanagedCall(GenTreeCall* call, CORINFO_SIG_INFO* sig, /* OUT */ CallArg** swiftErrorArg, /* OUT */ CallArg** swiftSelfArg);
void impPopArgsForUnmanagedCall(GenTreeCall* call, CORINFO_SIG_INFO* sig, /* OUT */ CallArg** swiftErrorArg);

#ifdef SWIFT_SUPPORT
void impAppendSwiftErrorStore(GenTreeCall* call, CallArg* const swiftErrorArg);
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1843,6 +1843,13 @@ regNumber CallArgs::GetCustomRegister(Compiler* comp, CorInfoCallConvExtension c
case WellKnownArg::DispatchIndirectCallTarget:
return REG_DISPATCH_INDIRECT_CALL_ADDR;
#endif

#ifdef SWIFT_SUPPORT
case WellKnownArg::SwiftSelf:
assert(cc == CorInfoCallConvExtension::Swift);
return REG_SWIFT_SELF;
#endif // SWIFT_SUPPORT

default:
break;
}
Expand Down Expand Up @@ -13091,6 +13098,8 @@ const char* Compiler::gtGetWellKnownArgNameForArgMsg(WellKnownArg arg)
case WellKnownArg::ValidateIndirectCallTarget:
case WellKnownArg::DispatchIndirectCallTarget:
return "cfg tgt";
case WellKnownArg::SwiftSelf:
return "swift self";
default:
return nullptr;
}
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4401,7 +4401,7 @@ enum class CFGCallKind

class CallArgs;

enum class WellKnownArg
enum class WellKnownArg : unsigned
Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Why this change? (Seems fine, just curious)

Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Mar 1, 2024

Choose a reason for hiding this comment

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

CallArg::m_wellKnownArg is 5 bits, and WellKnownArg::SwiftSelf has a value of 16 (also 5 bits). If m_wellKnownArg is signed, then we lose the most-significant bit, so comparisons like m_wellKnownArg == WellKnownArg::SwiftSelf fail.

{
None,
ThisPointer,
Expand All @@ -4419,6 +4419,7 @@ enum class WellKnownArg
R2RIndirectionCell,
ValidateIndirectCallTarget,
DispatchIndirectCallTarget,
SwiftSelf,
};

#ifdef DEBUG
Expand Down Expand Up @@ -4728,6 +4729,7 @@ class CallArg
CORINFO_CLASS_HANDLE GetSignatureClassHandle() { return m_signatureClsHnd; }
var_types GetSignatureType() { return m_signatureType; }
WellKnownArg GetWellKnownArg() { return m_wellKnownArg; }
void SetWellKnownArg(const WellKnownArg argType) { m_wellKnownArg = argType; }
bool IsTemp() { return m_isTmp; }
// clang-format on

Expand Down
38 changes: 29 additions & 9 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,9 @@ var_types Compiler::impImportCall(OPCODE opcode,
CORINFO_SIG_INFO calliSig;
NewCallArg extraArg;

// Swift calls may use special register types that require additional IR to handle,
// so if we're importing a Swift call, look for these types in the signature
// Swift calls that might throw use a SwiftError* arg that requires additional IR to handle,
// so if we're importing a Swift call, look for this type in the signature
CallArg* swiftErrorArg = nullptr;
CallArg* swiftSelfArg = nullptr;

/*-------------------------------------------------------------------------
* First create the call node
Expand Down Expand Up @@ -670,7 +669,7 @@ var_types Compiler::impImportCall(OPCODE opcode,

checkForSmallType = true;

impPopArgsForUnmanagedCall(call->AsCall(), sig, &swiftErrorArg, &swiftSelfArg);
impPopArgsForUnmanagedCall(call->AsCall(), sig, &swiftErrorArg);

goto DONE;
}
Expand Down Expand Up @@ -1840,8 +1839,7 @@ GenTreeCall* Compiler::impImportIndirectCall(CORINFO_SIG_INFO* sig, const DebugI

void Compiler::impPopArgsForUnmanagedCall(GenTreeCall* call,
CORINFO_SIG_INFO* sig,
/* OUT */ CallArg** swiftErrorArg,
/* OUT */ CallArg** swiftSelfArg)
/* OUT */ CallArg** swiftErrorArg)
{
assert(call->gtFlags & GTF_CALL_UNMANAGED);

Expand All @@ -1867,6 +1865,7 @@ void Compiler::impPopArgsForUnmanagedCall(GenTreeCall* call,

#ifdef SWIFT_SUPPORT
unsigned short swiftErrorIndex = sig->numArgs;
unsigned short swiftSelfIndex = sig->numArgs;

// We are importing an unmanaged Swift call, which might require special parameter handling
if (call->unmgdCallConv == CorInfoCallConvExtension::Swift)
Expand Down Expand Up @@ -1915,13 +1914,29 @@ void Compiler::impPopArgsForUnmanagedCall(GenTreeCall* call,
swiftErrorIndex = argIndex;
checkEntireStack = true;
}
// TODO: Handle SwiftSelf, SwiftAsync
else if ((strcmp(className, "SwiftSelf") == 0) &&
(strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
{
// We expect a SwiftSelf struct to be passed, not a pointer/reference
if (argIsByrefOrPtr)
{
BADCODE("Expected SwiftSelf struct, got pointer/reference");
}

if (swiftSelfIndex != sig->numArgs)
{
BADCODE("Duplicate SwiftSelf parameter");
}

swiftSelfIndex = argIndex;
}
// TODO: Handle SwiftAsync
}

// Don't need to reverse args for Swift calls
argsToReverse = 0;

// If using one of the Swift register types, check entire stack for side effects
// If using SwiftError*, check entire stack for side effects
if (checkEntireStack)
{
impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("Spill for swift calls"));
Expand Down Expand Up @@ -2006,7 +2021,12 @@ void Compiler::impPopArgsForUnmanagedCall(GenTreeCall* call,
assert(swiftErrorArg != nullptr);
*swiftErrorArg = &arg;
}
// TODO: SwiftSelf, SwiftAsync
else if (argIndex == swiftSelfIndex)
{
// Found the SwiftSelf arg
arg.SetWellKnownArg(WellKnownArg::SwiftSelf);
}
// TODO: SwiftAsync
#endif // SWIFT_SUPPORT

argIndex++;
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,8 @@ const char* getWellKnownArgName(WellKnownArg arg)
return "ValidateIndirectCallTarget";
case WellKnownArg::DispatchIndirectCallTarget:
return "DispatchIndirectCallTarget";
case WellKnownArg::SwiftSelf:
return "SwiftSelf";
}

return "N/A";
Expand Down Expand Up @@ -3179,7 +3181,9 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call)
}

// TODO-ARGS: Review this, is it really necessary to treat them specially here?
if (call->gtArgs.IsNonStandard(this, call, &arg) && arg.AbiInfo.IsPassedInRegisters())
// Exception: Lower SwiftSelf struct arg to GT_LCL_FLD
if (call->gtArgs.IsNonStandard(this, call, &arg) && arg.AbiInfo.IsPassedInRegisters() &&
(arg.GetWellKnownArg() != WellKnownArg::SwiftSelf))
{
Comment on lines +3184 to 3187
Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Why is this necessary? That doesn't seem right.

Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Likely we should instead insert a new arg in the importer that represents "self" as a primitive instead of keeping this as a struct argument. It being a struct argument is just an artifact of the design and it wouldn't work to keep it as a struct on all ABIs.

Copy link
Member

@jakobbotsch jakobbotsch Mar 1, 2024

Choose a reason for hiding this comment

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

Let's just keep it as is for now. I'm refactoring stuff for the struct work which should make this change simple to make.

Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Mar 1, 2024

Choose a reason for hiding this comment

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

Likely we should instead insert a new arg in the importer that represents "self" as a primitive instead of keeping this as a struct argument.

Yeah, the fact that the argument was being kept as TYP_STRUCT was causing asserts in later phases.

Let's just keep it as is for now. I'm refactoring stuff for the struct work which should make this change simple to make.

Sure thing.

flagsSummary |= argx->gtFlags;
continue;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/targetamd64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,6 @@
#define SWIFT_SUPPORT
#define REG_SWIFT_ERROR REG_R12
#define RBM_SWIFT_ERROR RBM_R12
#define SWIFT_SELF_REG REG_R13
#define REG_SWIFT_SELF REG_R13

// clang-format on
2 changes: 1 addition & 1 deletion src/coreclr/jit/targetarm64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,6 @@
#define SWIFT_SUPPORT
#define REG_SWIFT_ERROR REG_R21
#define RBM_SWIFT_ERROR RBM_R21
#define SWIFT_SELF_REG REG_R20
#define REG_SWIFT_SELF REG_R20

// clang-format on
2 changes: 0 additions & 2 deletions src/tests/Interop/Swift/SwiftSelfContext/SwiftSelfContext.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<!-- Swift interop is supported on Apple platforms only -->
<CLRTestTargetUnsupported Condition="'$(TargetsOSX)' != 'true' and '$(TargetsAppleMobile)' != 'true'">true</CLRTestTargetUnsupported>
<!-- Tracking issue: https://github.com/dotnet/runtime/issues/93631 -->
<CLRTestTargetUnsupported Condition="'$(RuntimeFlavor)' != 'mono'">true</CLRTestTargetUnsupported>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
Expand Down
3 changes: 0 additions & 3 deletions src/tests/issues.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@
<ExcludeList Include="$(XunitTestBinBase)/Interop/Swift/SwiftInvalidCallConv/*">
<Issue>https://github.com/dotnet/runtime/issues/93631</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/Interop/Swift/SwiftSelfContext/*">
<Issue>https://github.com/dotnet/runtime/issues/93631</Issue>
</ExcludeList>
</ItemGroup>

<!-- All Unix targets on all runtimes -->
Expand Down