Move return buffer handling from interop to the JIT #39294

jkoritzinsky · 2020-07-14T17:12:00Z

So far, when specific calling conventions have required different rules around return buffers than the managed calling conventions, the JIT has relied on the interop space to munge the IL stubs so the JIT doesn't have to handle the return buffer logic itself. Through the work for supporting thiscall in .NET Core 3.x and .NET 5, we've found that handling the return buffer logic in the interop space for anything more than the most trivial cases is prone to bugs and makes the logic hard to follow. In addition, handling the logic in the IL stubs precludes UnmanagedCallersOnly methods that might need return buffers that would be provided by the interop system from not needing an IL stub.

This PR moves the return buffer handling into the JIT so the interop space doesn't need to know when to create a return buffer nor does it need to know how to munge the IL to correctly create return buffers in the managed->native and native->managed directions.

Fixes #12375

The general logic of this PR goes as follows:

General return buffer handling

On Unix x86, always use a return buffer for structures. On Windows x86, use the JITs support for unwrapping trivial structs to enregister <=4-byte structs for non-thiscall scenarios. Add in EAX/EDX multireg return support for 8-byte structures on Windows x86 when not in the thiscall scenario.

Managed->Native

On Windows non-ARM, the JIT will generate a struct return buffer argument passed as per the platform ABI for thiscall.

Native->Managed

On platforms where we have a standard UMThunkStub (non-Windows x86), we pass the unmanaged calling convention to the JIT.

On Windows non-ARM, the JIT will generate a return buffer for thiscall methods.

Since Windows x86 has a special stub linker, we update the thiscall handling there. Since the current implementation tries to surface thiscall as stdcall for the majority of the thunk by swapping around arguments, I augment that support by adding support for copying an EAX/EDX multireg return value from the managed call to the native return buffer.

…tbuf arg after the `this` parameter, not before.

…hiscall on required platforms. Still need to reorder the emit so the arguments are emitted correctly.

… JIT can handle it.

…primitive-wrapper struct returning thiscall working.

…eems to optimize them correctly now. Also this enables the JIT to correctly handle native instance method calls that return single primitive field structs.

… since the arguments have already been reversed) on x86.

…a test in interop to validate.

…n for thiscall Reverse P/Invoke stubs, update the stub linker to handle the case where a return value would be an enregistered return on stdcall but is returned via a return buffer for thiscall.

…f-move-to-jit

…s a value type. Use a type handle instead of a method table. Don't check size for enums.

…f-move-to-jit

jkoritzinsky · 2020-07-17T17:50:01Z

Looks like there's more work I need to do to support the EAX/EDX multireg return on Windows x86. I'm locally seeing asserts about how the GCInfo emitting doesn't know how to handle multireg struct returns on Windows x86 Debug (which is possibly causing the libraries test failures). And there's the other assert on Windows x86 Checked when crossgenning, so I need to figure that out as well.

cc: @CarolEidt @echesakovMSFT if you could take a look when you have a chance and see if you can figure out what I'm missing/what I need to add or change to get this working, that would be great!

jkotas · 2020-07-17T20:00:13Z

I'm locally seeing asserts about how the GCInfo emitting doesn't know how to handle multireg struct returns on Windows x86 Debug

I would avoid changing managed calling convention in this change, in particular for x86. I would limit this change to just changing handling of unmanaged calling conventions in the JIT.

jkoritzinsky · 2020-07-17T20:03:08Z

Ok. I'll try to limit this change to unmanaged calls only.

jkoritzinsky · 2020-07-31T23:30:38Z

I've tried to move this to only being applied to unmanaged calls, but there are a few places where I don't think I can easily ask "is this an unmanaged call?" when the JIT asks "how do I return this struct?" and as a result I'm ending up with the runtime in a bad state.

Since I also don't know how to correctly emit the GCInfo for the alternative case of multireg returns on x86 on all platforms, I'm going to ask for help.

@dotnet/jit-contrib any suggestions on how to handle the fact that unmanaged calls on x86 need to support multireg returns for 8-byte structs without causing the JIT to fail because we don't support multireg returns on x86 in the GCInfo encoder? We can't just have interop tell the JIT that the 8-byte struct is a long since in the ThisCall case, 8 byte structs are returned in a return buffer but longs are returned in a register.

Signed-off-by: Jeremy Koritzinsky <[email protected]>

sandreenko · 2020-11-30T01:36:28Z

In #39294 (comment) we were discussing CorInfoUnmanagedCallConv and if should have CORINFO_UNMANAGED_CALLCONV_MANAGED there that is identical to CORINFO_UNMANAGED_CALLCONV_UNKNOWN, I can't respond to that thread so I am starting a new one, the current name still confuses me and hiding it under a define in Jit code does not look like a solution.
I think the questions are:

Does Jit need to distinguish MANAGED from UNKNOWN?
Do other Runtime parts need to distinguish them?
Why enum is called enum CorInfoUnmanagedCallConv, can it be called enum CorInfoCallConvExtension or something similar?
could it be enum class?

jkotas · 2020-11-30T02:35:42Z

Why enum is called enum CorInfoUnmanagedCallConv

For historic reasons, there are multiple (at least 4) partially overlapping calling convention enum definitions.

Notice that CorInfoUnmanagedCallConv is subset of CorInfoCallConv. My guess is whoever introduced CorInfoUnmanagedCallConv as a subset of CorInfoCallConv wanted to make it clear that getUnmanagedCallConv method on JIT/EE interface returns a subset of callconv values. If we were to change the name of CorInfoUnmanagedCallConv to CorInfoCallConvExtension and mixed managed calling convention into it, we would lose this clarity. I think we would be better off to just use CorInfoCallConv instead of introducing CorInfoCallConvExtension.

Does Jit need to distinguish MANAGED from UNKNOWN?

It does not. UNKNOWN is rejected by the JIT inside impCheckForPInvokeCall. Nothing in the JIT will see UNKNOWN except for this method.

I do not believe that CORINFO_UNMANAGED_CALLCONV_UNKNOWN is actually ever returned by JIT/EE interface. And if it was ever returned, the JIT side immediately rejects it as something that it does not understand. It makes it safe to use this value as sentinel inside the JIT internals.

Do other Runtime parts need to distinguish them?

Other runtime parts need to distinguish between unspecified unmanaged calling convention that is substituted with platform default and explicitly specified unmanaged calling convention. They use other calling convention enums for this some of the time (e.g. CorPinvokeMap).

could it be enum class?

If we want to switch the JIT/EE interface to use enum classes, it should be done as a separate style-only PR, accross all enums on JIT/EE interface. This PR is big enough as it is.

sandreenko · 2020-11-30T03:02:34Z

@jkotas got it, thank you.
Then I prefer what Jeremy suggested earlier: add a new jit internal enum for it (enum class CorInfoCallConvExtension) and use it instead of CorInfoUnmanagedCallConv.

sandreenko

It is hard to see the whole picture for such a big change, the small parts look close to a merge state.

I left a few comments/questions.

src/coreclr/src/jit/codegenxarch.cpp

sandreenko · 2020-11-29T23:44:40Z

src/coreclr/src/jit/compiler.cpp

@@ -693,7 +742,7 @@ var_types Compiler::getArgTypeForStruct(CORINFO_CLASS_HANDLE clsHnd,
    assert(structSize != 0);

 // Determine if we can pass the struct as a primitive type.
-// Note that on x86 we never pass structs as primitive types (unless the VM unwraps them for us).
+// Note that on x86 we only pass specific pointer-sized structs as primitives that the VM used to unwrap.


We will soon forget what VM used to unwrap, maybe delete an outdated reference and say something like:
// Note that on Windows we only pass specific pointer-sized structs that satisfy isTrivialPointerSizedStruct checks

Also, the body says:

On Unix x86, always use a return buffer for structures.

Is it still true? What does this method return for a trivial struct on x86 unix?

The native calling conventions always uses a return buffer for structures, even non-trivial ones, on x86 unix. For the managed calling convention, we'll continue to support the current system (4-byte structs returned in registers).

src/coreclr/src/jit/compiler.cpp

src/coreclr/src/jit/compiler.h

src/coreclr/src/jit/lclvars.cpp

sandreenko · 2020-11-30T02:02:18Z

src/coreclr/src/jit/lclvars.cpp

+    // Skip any user args that we've already processed.
+    assert(userArgsToSkip <= argSigLen);
+    argSigLen -= userArgsToSkip;
+    for (unsigned i = 0; i < userArgsToSkip; i++, argLst = info.compCompHnd->getArgNext(argLst))


as I see userArgsToSkip can be only 0 or 1, do we need a loop here?

The loop is moreso to future proof this so it works for any input, even though we currently only use 0 or 1. If you prefer, I can change this to be an if condition and assert that the value is 0 or 1.

src/coreclr/src/jit/morph.cpp

sandreenko · 2020-11-30T03:22:08Z

src/coreclr/src/jit/morph.cpp

@@ -2837,10 +2846,11 @@ void Compiler::fgInitArgInfo(GenTreeCall* call)
        {
            maxRegArgs = 0;
        }
-
+#ifdef UNIX_X86_ABI


why do we need this change?

This change was to preserve the previous behavior. We used to do this on both x86 platforms. For consistency with the previous behavior (since I don't have a unix x86 test setup and we don't run it in CI), I wanted to keep this block active for Unix x86.

src/coreclr/src/jit/morph.cpp

jkoritzinsky · 2020-11-30T19:23:56Z

If we use CorInfoCallConv, then we won't be able to extend the enum later to represent calling conventions defined using the "extensible calling convention" system we designed for function pointers. We're limited in the number of bits we can use in that enum since it's a direct representation of metadata. I'd prefer to use a different type than CorInfoCallConv so we can more easily add new calling conventions and extend the enum we use in this PR to represent the new "extensible calling convention" calling conventions along the EE-JIT border and inside the JIT.

Signed-off-by: Jeremy Koritzinsky <[email protected]>

…gedCallConv

…o-jit

jkoritzinsky · 2020-12-02T18:48:56Z

I've addressed all of the feedback excluding the conversation about "union vs side table". Does anyone have any more feedback for this PR before approval?

BruceForstall · 2020-12-02T18:53:41Z

Once all the PR feedback has been addressed and the outerloop test run is clean, I suggest you trigger basically every JIT and GC stress AzDO job.

Have you analyzed JIT throughput impact (if any is expected)? Verified no asm diffs (if none are expected), or expected asm diffs?

jkoritzinsky · 2020-12-02T18:56:39Z

I don't expect any JIT throughput impact. I've verified that the only asm diffs are the expected asm diffs (mentioned above that the only ones are on x86 and due to the change in struct normalization). I'll trigger the stress jobs once the PR jobs are done to try to not overload the queues.

jkoritzinsky · 2020-12-02T21:25:10Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2020-12-02T21:25:32Z

Azure Pipelines successfully started running 1 pipeline(s).

jkoritzinsky · 2020-12-03T18:12:02Z

I've run the outerloop pipelines. I've validated that the jitstress failures match the same failures in master. The gcstress+jitstress failures are a little harder to validate, but it looks to me like they're the same failures that exist in master as well.

sandreenko

LGTM, thanks for your patience during this massive change.

I have one more request, that could be done as a separate PR, I can open an issue if you don't have time for it right now:
https://github.com/dotnet/runtime/blob/master/docs/design/coreclr/botr/clr-abi.md does not have a description (or link) for x86/arm abi, so it would not be fair to ask you to write it fully, but could you please expand your examples from the PR header and put them there with additional comments and examples with accent on new managed/native differences?

Managed->Native
On Windows non-ARM, the JIT will generate a struct return buffer argument passed as per the platform ABI for thiscall.

please add what happens for windows arm and not for thiscall.

I think I would prefer just to see a banch of example, with different struct sizes, with/without special arguments/profiler attached etc with comments where each argument goes on each platform.

jkoritzinsky · 2020-12-03T19:58:10Z

I'll work on another change to update that doc with my learnings in another PR so I don't slow down merging this one in. Thanks!

jkoritzinsky added 18 commits July 7, 2020 12:01

Turn off interop-level system to enable thiscall handling

c17d786

Generate a return buffer for instance method callis.

8eecd35

On Windows non-arm32, unmanaged instance methods have their hidden re…

c06a040

…tbuf arg after the `this` parameter, not before.

Teach the JIT to generate a return buffer for reverse P/Invokes for t…

9448965

…hiscall on required platforms. Still need to reorder the emit so the arguments are emitted correctly.

Emit the native this argument before the return buffer when needed.

7a26d40

Remove isInstanceMethod support in the interop subsystem now that the…

a75f746

… JIT can handle it.

Add reverse P/Invoke tests for ThisCall.

4f09a4f

Fix x86 build.

e4aaf91

Add enum test.

3d66e53

Remove interop-specific handling of x86 thiscall and get non trivial-…

4eee458

…primitive-wrapper struct returning thiscall working.

Remove bashing the return type to int.

a7e0b52

Don't unwrap single primitive field structs on x86 any more. RyuJIT s…

8eb6846

…eems to optimize them correctly now. Also this enables the JIT to correctly handle native instance method calls that return single primitive field structs.

Pass the retbuf arg on the stack as the first argument (inserted last…

110c2ae

… since the arguments have already been reversed) on x86.

Remove extra newline

96336f5

Enable returning 8-byte structs in multiple registers on x86 and add …

48e3fce

…a test in interop to validate.

Since the x86 stub linker path emulates the stdcall calling conventio…

b83c60a

…n for thiscall Reverse P/Invoke stubs, update the stub linker to handle the case where a return value would be an enregistered return on stdcall but is returned via a return buffer for thiscall.

Remove unused code for manual return buffers from the interop subsystem.

f3bdf58

Merge branch 'master' of https://github.com/dotnet/runtime into retbu…

e7b8369

…f-move-to-jit

jkoritzinsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 14, 2020

jkoritzinsky added this to the 6.0.0 milestone Jul 14, 2020

jkoritzinsky added 4 commits July 14, 2020 10:56

Apply format patch.

5324b7e

Don't check IsUmanagedValueTypeReturnedByRef unless the methodtable i…

3c60ca2

…s a value type. Use a type handle instead of a method table. Don't check size for enums.

Merge branch 'master' of https://github.com/dotnet/runtime into retbu…

f23c291

…f-move-to-jit

Fix HasRetBuffArg check for ARM and ARM64

0a6790b

jkoritzinsky mentioned this pull request Aug 7, 2020

UnmanagedCallersOnlyAttribute returning non-primitive value types do not work #35928

Closed

sandreenko self-requested a review August 20, 2020 23:11

jkoritzinsky added 2 commits November 23, 2020 17:06

Add some comments around CorInfoUnmanagedCallConv

96081a0

Signed-off-by: Jeremy Koritzinsky <[email protected]>

CALLCONV_MANAGED=CALLCONV_UNKNOWN jit local

0ef4948

sandreenko reviewed Nov 30, 2020

View reviewed changes

jkoritzinsky added 9 commits November 30, 2020 12:01

PR feedback.

6ee4a82

Use new HasFixedRetBufArg method.

3c89d74

Fix formatting.

2ce4270

Fix compMethodReturnsRetBufAddr

fb00bc9

Signed-off-by: Jeremy Koritzinsky <[email protected]>

Create CorInfoCallConvExtension enum instead of reusing CorInfoUnmana…

8f9478a

…gedCallConv

Merge branch 'master' of github.com:dotnet/runtime into retbuf-move-t…

68d4081

…o-jit

Fix the unreachable code warnings.

e26c36d

Fix formatting

c1040aa

Use FEATURE_MULTIREG_RET instead of TARGET_* for XARCH case.

fa2f2aa

runfoapp bot mentioned this pull request Dec 3, 2020

FrameworkHiveSelection_GlobalHiveWithBetterMatch failed in CI #45510

Closed

sandreenko approved these changes Dec 3, 2020

View reviewed changes

jkoritzinsky merged commit b66138c into dotnet:master Dec 3, 2020

jkoritzinsky deleted the retbuf-move-to-jit branch December 3, 2020 19:58

This was referenced Dec 5, 2020

Enable non-blittable struct returns on UnmanagedCallersOnly #45625

Merged

Add a blurb on the x86 calling convention to clr-abi.md #45807

Merged

ghost locked as resolved and limited conversation to collaborators Jan 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move return buffer handling from interop to the JIT #39294

Move return buffer handling from interop to the JIT #39294

jkoritzinsky commented Jul 14, 2020

jkoritzinsky commented Jul 17, 2020

jkotas commented Jul 17, 2020

jkoritzinsky commented Jul 17, 2020

jkoritzinsky commented Jul 31, 2020

sandreenko commented Nov 30, 2020

jkotas commented Nov 30, 2020 •

edited

Loading

sandreenko commented Nov 30, 2020

sandreenko left a comment

sandreenko Nov 29, 2020

sandreenko Nov 29, 2020

jkoritzinsky Nov 30, 2020

sandreenko Nov 30, 2020

jkoritzinsky Nov 30, 2020

sandreenko Nov 30, 2020

jkoritzinsky Nov 30, 2020

jkoritzinsky commented Nov 30, 2020

jkoritzinsky commented Dec 2, 2020

BruceForstall commented Dec 2, 2020

jkoritzinsky commented Dec 2, 2020

jkoritzinsky commented Dec 2, 2020

azure-pipelines bot commented Dec 2, 2020

jkoritzinsky commented Dec 3, 2020

sandreenko left a comment

jkoritzinsky commented Dec 3, 2020

Move return buffer handling from interop to the JIT #39294

Move return buffer handling from interop to the JIT #39294

Conversation

jkoritzinsky commented Jul 14, 2020

jkoritzinsky commented Jul 17, 2020

jkotas commented Jul 17, 2020

jkoritzinsky commented Jul 17, 2020

jkoritzinsky commented Jul 31, 2020

sandreenko commented Nov 30, 2020

jkotas commented Nov 30, 2020 • edited Loading

sandreenko commented Nov 30, 2020

sandreenko left a comment

Choose a reason for hiding this comment

sandreenko Nov 29, 2020

Choose a reason for hiding this comment

sandreenko Nov 29, 2020

Choose a reason for hiding this comment

jkoritzinsky Nov 30, 2020

Choose a reason for hiding this comment

sandreenko Nov 30, 2020

Choose a reason for hiding this comment

jkoritzinsky Nov 30, 2020

Choose a reason for hiding this comment

sandreenko Nov 30, 2020

Choose a reason for hiding this comment

jkoritzinsky Nov 30, 2020

Choose a reason for hiding this comment

jkoritzinsky commented Nov 30, 2020

jkoritzinsky commented Dec 2, 2020

BruceForstall commented Dec 2, 2020

jkoritzinsky commented Dec 2, 2020

jkoritzinsky commented Dec 2, 2020

azure-pipelines bot commented Dec 2, 2020

jkoritzinsky commented Dec 3, 2020

sandreenko left a comment

Choose a reason for hiding this comment

jkoritzinsky commented Dec 3, 2020

jkotas commented Nov 30, 2020 •

edited

Loading