JIT: Support Swift error handling for reverse P/Invokes

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.

[amanasifkhalid]

.NET methods called from Swift update error handling state through a SwiftError* argument; when returning from .NET to Swift, the error value pointed to by the SwiftError* must be loaded into the error register. We use the following IR to support this:

• The JIT creates a SwiftError "pseudo-local", and transforms all uses of the SwiftError* argument into GT_LCL_ADDR nodes of the pseudo-local.
• When generating IR for the reverse P/Invoke exit point, we create a GT_SWIFT_ERROR_RET node for loading the SwiftError pseudo-local's value into the error register before returning.

I've added a new test for exercising .NET callbacks with error handling to SwiftErrorHandling.

cc @jkoritzinsky@kotlarmilos, @jakobbotsch PTAL

[dotnet-policy-service]

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

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[kotlarmilos]

LGTM!

[amanasifkhalid]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

[amanasifkhalid]

This change touches some hot paths (such as during importation), so there is some TP impact on Unix x64, though oddly not on arm64.

[kotlarmilos]

I'm seeing some weird interaction with the SwiftSelf parameter in the reverse P/Invoke test, where its value in the .NET callback is different from the value it was initialized with. The error handling in the callback still works correctly, in that the SwiftError value is set to the SwiftSelf value, but the assert in the test fails because the address the SwiftError was set to isn't correct. I run into the same issue even if I replace the SwiftSelf parameter with an IntPtr containing &expectedValue. I've simplified this test to just hard-code the SwiftError value in the callback so we know the error register is being propagated down the call stack; locally, this works with all the stress modes.

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

Once I've verified this passes in CI, I'll disable this test for Mono. @kotlarmilos is there any way to disable individual tests for Mono, or do I have to make a separate project in src/tests/Interop/Swift for this new test, and disable it for Mono in issues.targets? Thanks!

Try adding the SkipOnMono attribute to the test case.

[amanasifkhalid]

According to the Swift calling convention, when passing closures, there is an implicit argument immediately after that is a pointer to the closure context, and it gets stored in the call context register (swiftself). In your test, the address of swifterror gets stored into swiftself, which may lead to unexpected behavior.

I see, thanks for pointing that out -- that explains why the diff in the initial and updated SwiftSelf value was always the same. I guess it makes sense for the SwiftErrorHandling tests to test SwiftError in isolation, anyway.

Try adding the SkipOnMono attribute to the test case.

Thanks! I'll update in the next review iteration.

[jakobbotsch]

Seems like it would be cleaner to return the GenTree* node from impPopArgsForUnmanagedCall and remove the arg in there instead.

[jakobbotsch]

Seems odd given the assert.

[amanasifkhalid]

Sorry I meant to remove the assert; doing so seems to fix the jitstressregs failure locally.

I suppose I could remove REG_SWIFT_ERROR from the register mask in the caller, but doing it here means we'll do the removal only if REG_SWIFT_ERROR was ever set in the first place.

[jakobbotsch]

Perhaps the caller shouldn't be passing it? Seems we could unify a bit of code by replacing some of the RBM_INT_CALLEE_SAVED occurrences in the backend with a function that takes the error return into account.

[amanasifkhalid]

That sounds better. Do you think it would make sense for this function to be a member of RegSet -- something like RegSet::rsGetModifiedIntCalleeSavedRegsMask -- since RegSet already has a Compiler* member variable to check for lvaSwiftErrorArg? Or would it be confusing if RegSet::rsGetModifiedIntCalleeSavedRegsMask sometimes doesn't include REG_SWIFT_ERROR, which is technically a callee-save register, even if it was modified?

[jakobbotsch]

I think it would be best to follow the same pattern as get_RBM_FLT_CALLEE_TRASH -- it has the same flavor of "sometimes needs to be determined based on compilation information", and RBM_FLT_CALLEE_TRASH is defined to call that function. It will ensure that everyone gets the consistent view, but will of course result in a much less localized change. If it ends up requiring too many changes or being costly then I would be fine with what you suggest.

[amanasifkhalid]

Thanks for the suggestion! I gave that a try, and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable; in particular, we use RBM_INT_CALLEE_SAVED in some static functions where we don't have access to the current Compiler's state, so we can't easily figure out whether to include RBM_SWIFT_ERROR in the mask. Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think RegSet provides a decent abstraction, though to make its usage consistent across the JIT, I had to make some trivial changes to architectures irrelevant to this PR.

[jakobbotsch]

Ok, it seems fine to me for now. Some notes...

and I think we use RBM_INT_CALLEE_SAVED in too many places to make this approach manageable

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

Also, I think the additional check for lvaSwiftErrorArg would've been more expensive than necessary, since we check RBM_INT_CALLEE_SAVED (directly and indirectly, via other macros) in places irrelevant to method prolog/epilog generation.

I think we would design it similar to get_RBM_FLT_CALLEE_TRASH -- the mask is stored in Compiler, so it's just returning the field. There is no dynamic logic to compute it on every invocation.

[amanasifkhalid]

This doesn't exactly fill me with confidence. It means there are many places getting the wrong and inconsistent view after this PR. Are we sure that none of those need to be updated for correctness sake? If we hit a bug tail around this we should reconsider.

This might be naive, but I think it's ok if we continue to think of REG_SWIFT_ERROR as callee-save elsewhere in the JIT, and limit the error register-specific quirks to prolog/epilog generation. We use RBM_INT_CALLEE_SAVED in the definition of RBM_ALLINT as well, so to ensure the latter includes REG_SWIFT_ERROR, we'd probably need to save the "normal" value of RBM_INT_CALLEE_SAVED to another macro -- the fact that this new macro would only be semantically different from RBM_INT_CALLEE_SAVED on Swift platforms seems messy. We'd also have to use this duplicated macro in the various static asserts that use RBM_INT_CALLEE_SAVED without access to compiler state.

The benefit of the current approach is its behavior is limited in scope. It might be too limited to the point where we are incorrectly treating REG_SWIFT_ERROR as callee-save (though I haven't run into that yet), but that seems easier to debug than the opposite scenario of making RBM_INT_CALLEE_SAVED stateful.

[amanasifkhalid]

jitformat is insistent on this odd spacing:

 TEMP_MAX_SIZE = FP_REGSIZE_BYTES,
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
#else // !FEATURE_SIMD
TEMP_MAX_SIZE = sizeof(double),
#endif // !FEATURE_SIMD
TEMP_SLOT_COUNT = (TEMP_MAX_SIZE / sizeof(int))
};

I'll push a fix after approval.

Edit: Bruce just merged in a clang-format/clang-tidy update, so maybe this is fixed...

[amanasifkhalid]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Do we need to potentially adjust swiftSelfIndex here? Or perhaps instead delay the removal until after the loop below?

[amanasifkhalid]

Good catch: If the SwiftSelf arg comes after the SwiftError* arg, then yes, we do need to decrement it. It seems easier to delay the removal. Fixed.

[jakobbotsch]

Can you verify that something somewhere marks this local as either do-not-enreg or address exposed? I don't immediately see where we do that in the JIT when something has implicit uses. Basically, I don't understand for example why liveness doesn't get rid of stores to this local because it cannot see any uses of it. Can you share a jitdump of one of the tests just so I can understand why that doesn't happen?

[amanasifkhalid]

Sure thing. I was able to remove the call to lvaSetVarAddrExposed here because fgMarkAddressExposedLocals seems to already do this in LocalAddressVisitor::EscapeAddress for each store to the pseudolocal before running liveness. Here's a JitDump for one of the callbacks (see line 1201, etc.).

[jakobbotsch]

I think we are getting "lucky" there. The JIT is spilling the address of the local to a temp (not really very optimal, we ought to improve this). It only does that because of the constructor call on the RHS of the assignment, and as a consequence we end up address exposing it. If you changed the code slightly, for example to

varx=newSwiftError(...);GC.KeepAlive(null);// avoid Roslyn optimizing it back to the pattern*error=x;

then I would expect that you will hit the issue I describe where nothing marks it as address exposed and we end up removing the store.

[amanasifkhalid]

I tried this, and you're correct that we no longer mark it as address exposed, but we don't end up removing the store -- I think this is because we mark all GT_LCL_ADDR nodes pointing to the pseudolocal as having side effects (perhaps that's what is forcing the JIT to spill the address to a temp?). I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation. My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

I added an updated dump to the gist with the above code pattern used in ConditionallySetErrorTo21 (though only in the true branch of the if statement).

[amanasifkhalid]

I'm fine with going back to explicitly marking the pseudolocal as address exposed upon creation

This, along with removing the side effects on the GT_LCL_ADDR nodes of the pseudolocal, improves codegen. For example, before:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06,T01] ( 2, 2 ) long -> rdi "impAppendStmt"
; V07 tmp4 [V07 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
; V09 tmp6 [V09 ] ( 2, 1 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "field V03.<Value>k__BackingField (fldOffset=0x0)" P-DEP
;* V10 tmp7 [V10,T03] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
lea rdi, [rbp-0x10]
xor eax, eax
mov qword ptr [rdi], rax
;; size=9 bbWeight=0.50 PerfScore 0.88
G_M3386_IG05: ;; offset=0x003F
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x004C
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 83, prolog size 27, PerfScore 16.58, instruction count 26, allocated bytes for code 83 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

After:

; Assembly listing for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Unix
; FullOpts code
; optimized code
; rbp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0 [V00 ] ( 0, 0 ) long -> zero-ref single-def
; V01 arg1 [V01,T00] ( 3, 3 ) int -> rbx single-def
;* V02 loc0 [V02 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op <System.Runtime.InteropServices.Swift.SwiftError>
; V03 tmp0 [V03 ] ( 3, 2 ) struct ( 8) [rbp-0x10] do-not-enreg[XS] must-init addr-exposed "SwiftError pseudolocal" <System.Runtime.InteropServices.Swift.SwiftError>
;# V04 OutArgs [V04 ] ( 1, 1 ) struct ( 0) [rsp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V05 tmp2 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp" <System.Runtime.InteropServices.Swift.SwiftError>
; V06 tmp3 [V06 ] ( 3, 3 ) struct (16) [rbp-0x20] do-not-enreg[XS] must-init addr-exposed "Reverse Pinvoke FrameVar"
;* V07 tmp4 [V07,T01] ( 0, 0 ) long -> zero-ref single-def "field V02.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;* V08 tmp5 [V08,T02] ( 0, 0 ) long -> zero-ref single-def "field V05.<Value>k__BackingField (fldOffset=0x0)" P-INDEP
;
; Lcl frame size = 24
G_M3386_IG01: ;; offset=0x0000
push rbp
push rbx
sub rsp, 24
lea rbp, [rsp+0x20]
vxorps xmm8, xmm8, xmm8
vmovdqa xmmword ptr [rbp-0x20], xmm8
xor eax, eax
mov qword ptr [rbp-0x10], rax
mov ebx, edi
;; size=29 bbWeight=1 PerfScore 6.58
G_M3386_IG02: ;; offset=0x001D
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER
test ebx, ebx
je SHORT G_M3386_IG04
;; size=13 bbWeight=1 PerfScore 2.75
G_M3386_IG03: ;; offset=0x002A
xor rdi, rdi
mov qword ptr [rbp-0x10], 21
jmp SHORT G_M3386_IG05
;; size=12 bbWeight=0.50 PerfScore 1.62
G_M3386_IG04: ;; offset=0x0036
xor edi, edi
mov qword ptr [rbp-0x10], rdi
;; size=6 bbWeight=0.50 PerfScore 0.62
G_M3386_IG05: ;; offset=0x003C
lea rdi, [rbp-0x20]
call CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT
mov r12, qword ptr [rbp-0x10]
;; size=13 bbWeight=1 PerfScore 2.50
G_M3386_IG06: ;; offset=0x0049
add rsp, 24
pop rbx
pop rbp
ret ;; size=7 bbWeight=1 PerfScore 2.25
; Total bytes of code 80, prolog size 27, PerfScore 16.33, instruction count 25, allocated bytes for code 80 (MethodHash=763af2c5) for method ErrorHandlingTests:ConditionallySetErrorTo21(ulong,int) (FullOpts)
; ============================================================

[jakobbotsch]

Ah, that makes sense. Yeah, the side effect flags on the GT_LCL_ADDR shouldn't be necessary,

My hope is that is short-lived anyway, once I open a PR for GT_SWIFT_ERROR_RET.

Do you plan to do the work? To be honest looking at the codegen there it really doesn't feel all that bad compared to all the other things we end up doing for the reverse pinvokes anyway (like the helper calls), so I'd be perfectly fine with just leaving it like this.

[amanasifkhalid]

I have some of the implementation saved locally, so I'm interested in trying it out; I think it can be done without polluting the JIT with too many weird edge cases.

[jakobbotsch]

The loop above removes/inserts arguments, so this part still has to happen before the loop above.

[jakobbotsch]

LGTM! Thanks for addressing all my feedback.

[amanasifkhalid]

LGTM! Thanks for addressing all my feedback.

Thanks for all the reviews!

[amanasifkhalid]

SPMI isn't finished running yet, but TP diffs look pretty small. I'm gonna merge to unblock #100344.