Support frozen struct returns for Swift calls

[jakobbotsch]

Adds support for pinvokes to Swift functions that return frozen structs in multiple registers. This turned out to be simpler than I expected on the JIT side; there is a small change necessary in genMultiRegStoreToLocal to take into account that the Swift fields are going into offsets that don't necessarily correspond to the register sizes (we already DNER the cases where things don't work out, it seems).

Also adds 100 tests.

The support is complicated by the fact that Swift calls take the ret buffer in rax on x64. This requires some VM side changes to avoid using rax in the interop thunks.

[dotnet-policy-service]

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

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @amanasifkhalid (JIT parts) @jkoritzinsky @jkotas (VM thunk changes)

FYI @kotlarmilos @matouskozak

I tested with 5000 tests both locally and in CI and they passed on both osx-arm64 and osx-x64. I reduced the set to 100 tests to be checked in.

[jakobbotsch]

This sequence has a superfluous rex prefix bit set to make sure the unwinder picks it up as an external tailcall:

runtime/src/coreclr/vm/amd64/excepamd64.cpp

Lines 368 to 378 in 2193467

	else if (((TempOpcode & 0xf8) == AMD64_SIZE64_PREFIX)
	&& (NextByte[1] == AMD64_JMP_IND_OP)
	&& (NextByte[2] & 0x38) == AMD64_JMP_IND_RAX)
	{
	//
	// This is an indirect jump opcode: 0x48 0xff /4. The 64-bit
	// flag (REX.W) is always redundant here, so its presence is
	// overloaded to indicate a branch out of the function - a tail
	// call.
	//
	// Such an opcode is an unambiguous epilogue indication.

(I wasn't totally sure whether it's necessary or not for this thunk, but the existing TAILJMP_RAX also has a superfluous rex prefix)

[jkotas]

This is Windows unwinder convention. I do not think it is applicable outside Windows.

[jkotas]

clang does not seem to generate a special jmp for tail calls on Unix:https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:36,endLineNumber:5,positionColumn:36,positionLineNumber:5,selectionStartColumn:36,selectionStartLineNumber:5,startColumn:36,startLineNumber:5),source:'extern+int+foo(int+x)%3B%0A%0Aextern+int+bar(int+x)%3B%0A%0Aint+foo(int+x)+%7B+return+bar(x%2B1)%3B+%7D'),l:'5',n:'0',o:'C%2B%2B+source+%231',t:'0')),k:48.26193742510047,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:compiler,i:(compiler:clang_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:2,lang:c%2B%2B,libs:!(),options:'-O3',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(trunk)+(Editor+%231)',t:'0')),header:(),k:51.73806257489954,l:'4',n:'0',o:'',s:0,t:'0')),l:'2',n:'0',o:'',t:'0')),version:4

[jakobbotsch]

I'll replace it with just jmp r10. I guess the TAILJMP_RAX uses in other non-Windows thunks can be cleaned up then.

Is there a difference between managed and native code here? The JIT is also generating these tailcalls with the extraneous rex prefixes. Can they be avoided outside Windows?

[jkotas]

Regular CoreCLR uses Windows unwinder outside Windows, so the JIT has to follow the Windows unwinder conventions there as well.

If we cared enough, we can skip the prefix for native AOT. We use native unwinder for native AOT so Windows unwinder conventions are not applicable outside Windows.

[amanasifkhalid]

JIT changes LGTM. Thanks!

[amanasifkhalid]

Since this isn't on a hot path, and we have at most 4 return registers for a Swift call, I'm guessing it's not worth caching the return registers used so we can skip this loop? Though we do call GetABIReturnReg while looping over all return register indices in a few places...

[jakobbotsch]

Ideally we would keep all the registers and also the offsets as part of this type, from a simple code hygiene standpoint, but increasing its size increases the size of GenTreeCall which increases the size of all large nodes. I think it would be good to clean this up to make the additional data for ReturnTypeDesc allocated only when necessary, but that's a separate change.

I don't think there's anything to be concerned about wrt. throughput here.

[amanasifkhalid]

MAX_RET_REG_COUNT is already 4 on ARM64, but it might be helpful to the reader to static_assert that here.

[jakobbotsch]

I added a static assert.

[jakobbotsch]

I wonder if we have a problem around CORINFO_HELP_PINVOKE_CALLI, in particular if we end up using a jump stub to call it that clobbers rax. We may need to ensure the JIT materializes its full address in the codegen for Swift calls with return buffers.

Edit: I guess not since that helper call is just going to use the regular managed calling convention.

[jakobbotsch]

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

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jkotas]

I wonder if we have a problem around CORINFO_HELP_PINVOKE_CALLI

The current design of CORINFO_HELP_PINVOKE_CALLI and vasig cookies is suboptimal. It leaves perf on the table and it is unfriendly to native AOT. We use a different approach in native AOT (look for convertPInvokeCalliToCall).

If you are running into troubles with CORINFO_HELP_PINVOKE_CALLI, it would be best to get rid of it and change the EE to generate a stub with slightly different shape, same as what we do in native AOT.

[jakobbotsch]

The current design of CORINFO_HELP_PINVOKE_CALLI and vasig cookies is suboptimal. It leaves perf on the table and it is unfriendly to native AOT. We use a different approach in native AOT (look for convertPInvokeCalliToCall).

If you are running into troubles with CORINFO_HELP_PINVOKE_CALLI, it would be best to get rid of it and change the EE to generate a stub with slightly different shape, same as what we do in native AOT.

I'll keep that in mind. I don't think we're going to see issues around it, however.