JIT: Add basic support for Swift reverse pinvokes by jakobbotsch · Pull Request #100018 · dotnet/runtime

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch · 2024-03-20T14:14:48Z

Adds very rudimentary support for Swift reverse pinvokes. Since Swift function pointers are based on SwiftSelf also adds basic support for the SwiftSelf parameter. Structs are not supported yet, and neither is SwiftError.

Adds 10 simple tests just so that we have something. I'm going to regenerate these (and probably increase the number to 100) once we have the struct support.

The SwiftSelf parameter is not enregisterable because it is passed in a register that genFnPrologCalleeRegArgs does not handle. I have other plans to rewrite this function in the future (#99286 (comment)), so I didn't want to bother spending time on it here (it is not a trivial extension given how it works).

jakobbotsch · 2024-03-20T14:24:52Z

-
- regType = compiler->GetEightByteType(structDesc, nCnt);
-#ifdef DEBUG
- regType = compiler->mangleVarArgsType(regType);
- assert(genMapRegNumToRegArgNum((nCnt == 0 ? regNum : otherRegNum), regType) != (unsigned)-1);
-#endif // DEBUG


I have removed this assert in a couple of places because I don't think it is asserting anything meaningful. The existence and mapping of genMapRegNumToRegArgNum is really an internal detail of how genFnPrologCalleeRegArgs works, and I expect to be able to remove it once that function is rewritten.

jakobbotsch · 2024-03-20T16:36:59Z

cc @dotnet/jit-contrib PTAL @amanasifkhalid

FYI @kotlarmilos @matouskozak @jkurdek

kotlarmilos · 2024-03-20T16:57:50Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


How does this get returned to the C# side?

The pinvokes in C# land are passing pointers to the ExceptionDispatchInfo as "self", and then check whether the it was set by this code after the return.

kotlarmilos · 2024-03-20T17:00:02Z

+ if ((strcmp(className, "SwiftSelf") == 0) && (strcmp(namespaceName, "System.Runtime.InteropServices.Swift") == 0))
+ {
+ LclVarDsc* varDsc = varDscInfo->varDsc;
+ varDsc->SetArgReg(REG_SWIFT_SELF);


Does this mean that the reg value is loaded into the SwiftSelf argument?

The JIT can directly reference and use IL locals in its internal IR, and each of the parameter locals are marked with information about how they're passed (in a register or on stack). That's what the code here is doing.
It's ultimately up to the register allocator to decide what register every local is allocated into. For parameter locals LSRA will prefer to keep it in the same register that it was passed in, but it might also be allocated to another register (in which case a move from the initial register is needed in the prolog) or to stack (in which case a store to the stack frame is needed).
The PR here doesn't allow enregistering SwiftSelf so we will always end up keeping it on the stack frame. That's because the case where there are conflicts in the move graph is not yet handled for this register (e.g. we can have cases where the SwiftSelf register x20 needs to go to x0 and x0 needs to go to x20). It needs a bit of work but I was expecting to work on the relevant logic due to another issue, so I will handle both of these when I do that work.

Ok, thanks for info.

amanasifkhalid

LGTM, just a few comments. Thanks!

amanasifkhalid · 2024-03-20T16:50:31Z

+ }
+ catch (Exception ex)
+ {
+ *(ExceptionDispatchInfo*)self.Value = ExceptionDispatchInfo.Capture(ex);


Once SwiftError works with reverse Pinvoke, I assume we're going to update this error handling to use the SwiftError arg instead, right?

Yeah, I think that would make sense.

Although handling it through SwiftError will require allocating a GCHandle, so the current approach will probably end up being simpler.

amanasifkhalid · 2024-03-20T16:51:20Z

@@ -0,0 +1,482 @@
+#pragma warning disable CS8500


Do we need the // Licensed to the .NET Foundation under one or more agreements. header comments at the top of these new files?

Good catch, I should add those.

I think some of the other interop tests like SwiftAbiStress are missing it too, but feel free to address that in a follow-up PR.

amanasifkhalid · 2024-03-20T17:01:45Z

 // mechanism passes the address of the return address to a runtime helper
 // where it is used to detect tail-call chains.
 unsigned lvaRetAddrVar;
+ unsigned lvaSwiftSelfArg;


Since all the places where we check lvaSwiftSelfArg are under #ifdef SWIFT_SUPPORT, should we ifdef this, and where we initialize it in Compiler::lvaInit?

Yeah, I'll do that.

amanasifkhalid · 2024-03-20T17:06:39Z

 CORINFO_ARG_LIST_HANDLE varList,
 CORINFO_SIG_INFO* varSig);

+ bool lvaInitSpecialSwiftParam(InitVarDscInfo* varDscInfo, CorInfoType type, CORINFO_CLASS_HANDLE typeHnd);


Based on the naming of this method, I guess we'll be using this to do the SwiftError* type lookup too...

Yeah, I was thinking that same logic could live here.

amanasifkhalid · 2024-03-20T17:07:13Z

 }

+#ifdef SWIFT_SUPPORT
+ if ((compiler->lvaSwiftSelfArg != BAD_VAR_NUM) && ((intRegState.rsCalleeRegArgMaskLiveIn & RBM_SWIFT_SELF) != 0))


Are there situations where lvaSwiftSelfArg is valid, but the reg mask doesn't include the Swift self reg? If not, maybe we should assert that the reg mask includes it if lvaSwiftSelfArg is set.

Yes, if lvaSwiftSelfArg is dead on entry then it shouldn't be in the live-in set.

amanasifkhalid · 2024-03-20T17:24:23Z

It might be a good idea to run jitstressregs (or a similar pipeline) on this PR

jakobbotsch · 2024-03-20T17:53:11Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-20T17:53:35Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-03-20T20:08:32Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

jakobbotsch · 2024-03-21T11:37:28Z

Looks like jitstressregs is failing with the assert genRegMask(argDsc->GetArgReg()) & (RBM_ARG_REGS) in lsrabuild.cpp (log)

Pushed some commits to fix this and address the rest of the feedback.

jakobbotsch · 2024-03-21T12:37:56Z

/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2024-03-21T12:38:13Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-03-21T17:10:52Z

Failures are #100035, #99810, #100047

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

jakobbotsch added 11 commits March 19, 2024 13:11

Add test
9a9a052

Support SwiftSelf
bfd4e28

Backend handling
53d00a6

Fix arm64 build
ea5c172

Add missing handling of DNER reason
4fd5964

Fix assert
ea32187

Fix build
e01d190

Remove some unnecessarily aggressive asserts
40d998a

Expand assert
4c64049

Reduce tests
7d43f3c

Disable tests on Mono
49826a8

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2024

dotnet-policy-serviceBot assigned jakobbotsch Mar 20, 2024

Regenerate tests
d65c4c3

jakobbotsch commented Mar 20, 2024
View reviewed changes

jakobbotsch marked this pull request as ready for review March 20, 2024 16:36

jakobbotsch requested a review from amanasifkhalid March 20, 2024 16:37

kotlarmilos reviewed Mar 20, 2024
View reviewed changes

amanasifkhalid approved these changes Mar 20, 2024
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2024
Tracking issue for CI build timeouts #76454
Closed

jakobbotsch added 3 commits March 21, 2024 12:19

Factor int parameter register validation
3e76803

Add missing license headers
21d1c52

More feedback
b4b73d7

Run jit-format
cb44f0b

This was referenced Mar 21, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed
"error: out of range pc-relative fixup value" on linux-armel checked CoreCLR_NonPortable #100047
Closed

jakobbotsch merged commit de774ff into dotnet:mainMar 21, 2024

jakobbotsch deleted the swift-reverse-pinvoke branch March 21, 2024 17:12

jkoritzinsky mentioned this pull request Mar 21, 2024
Runtime support for Swift Interop in .NET 9 #93631
Closed
42 tasks

github-actionsBot locked and limited conversation to collaborators Apr 21, 2024

Uh oh!

Conversation

jakobbotsch commented Mar 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Mar 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid commented Mar 20, 2024

Uh oh!

jakobbotsch commented Mar 20, 2024

Uh oh!

azure-pipelinesBot commented Mar 20, 2024

Uh oh!

amanasifkhalid commented Mar 20, 2024

Uh oh!

jakobbotsch commented Mar 21, 2024

Uh oh!

jakobbotsch commented Mar 21, 2024

Uh oh!

azure-pipelinesBot commented Mar 21, 2024

Uh oh!

jakobbotsch commented Mar 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jakobbotsch commented Mar 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Mar 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

jakobbotsch commented Mar 20, 2024•
edited
Loading

jakobbotsch commented Mar 20, 2024•
edited
Loading

jakobbotsch commented Mar 20, 2024•
edited
Loading

jakobbotsch commented Mar 20, 2024•
edited
Loading