-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move return buffer handling from interop to the JIT #39294
Changes from 62 commits
c17d786
8eecd35
c06a040
9448965
7a26d40
a75f746
4f09a4f
e4aaf91
3d66e53
4eee458
a7e0b52
8eb6846
110c2ae
96336f5
48e3fce
b83c60a
f3bdf58
e7b8369
5324b7e
3c60ca2
f23c291
0a6790b
925ec02
47553e5
779708a
4b8870b
6c56bc8
3897473
426b0f3
cc73336
a825b8a
9c0cbb1
63507b7
b72dcb4
1262c27
ef2adac
5c2a9bc
dd44e32
1056c44
a0218a1
47bf6aa
a1bb21c
254063b
71adacb
195713b
8833b06
7e2c1f9
382113f
bd4022a
c4acc94
1694da8
5dd9031
7182ad0
e329242
94f6a1d
cfce1d7
0368407
a4286f3
bae4165
77d6bf6
9e965f8
c28925e
e5212cb
e020165
90cfcef
41b21a5
f6eeddf
71c9032
b334b37
28b1d48
aab5ca0
96081a0
0ef4948
6ee4a82
3c89d74
2ce4270
fb00bc9
8f9478a
68d4081
e26c36d
c1040aa
fa2f2aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -512,6 +512,56 @@ bool Compiler::isSingleFloat32Struct(CORINFO_CLASS_HANDLE clsHnd) | |
} | ||
#endif // ARM_SOFTFP | ||
|
||
#ifdef TARGET_X86 | ||
//--------------------------------------------------------------------------- | ||
// isTrivialPointerSizedStruct: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: I know you don't want to change the managed calling convention here but could you please clarify which decisions are temporary here. For example. on x64 we pass 8 bytes struct in a reg even if it has several fields, but we restrict x86 to 1 field, is it a temporary solution? The same question about 8 byte struct, would they be passed as regs in managed calling conv in the future changes on x86? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of these decisions can be changed in a future PR if we're willing to take the R2R compat break. To my knowledge, changing the calling convention breaks ready-to-run compatibility, so I didn't want to do that in this PR if I could avoid it. |
||
// Check if the given struct type contains only one pointer-sized integer value type | ||
// | ||
// Arguments: | ||
// clsHnd - the handle for the struct type | ||
jkoritzinsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// | ||
// Return Value: | ||
// true if the given struct type contains only one pointer-sized integer value type, | ||
// false otherwise. | ||
// | ||
|
||
jkoritzinsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) | ||
jkoritzinsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
assert(info.compCompHnd->isValueClass(clsHnd)); | ||
if (info.compCompHnd->getClassSize(clsHnd) != TARGET_POINTER_SIZE) | ||
{ | ||
return false; | ||
} | ||
for (;;) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a lot of back and forth with the EE; would it make sense to do all this work over on the jit interface side? (I'm guessing we rarely iterate in this loop, but still...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's very unlikely we'll iterate through this loop more than once in my experience, but I can move this to the other side of the ee-jit interface if you think that's better for perf. I was trying to avoid the hassle of versioning the jit-ee interface, but I'm fine doing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be fine if you just measured how often we have to iterate (over say PMI FX) and just confirmed it is truly rare. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've run the PMI FX diff as you recommended and confirmed that in the whole framework we run the loop body at most twice. We run the loop body twice in 557 cases (551 cases are in Roslyn, the remaining 6 are in System.Reflection.Metadata), and we run the loop body only once in 102592 cases. So, we run the loop body only once in 0.5% of cases if we include the Roslyn assemblies in Core_Root, or approximately 0.005% of cases if we exclude the Roslyn assemblies. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we only make it past the first checks in the function (the pointer-sized check and the first "is value type with one field" check) 19% of the time. |
||
{ | ||
// all of class chain must be of value type and must have only one field | ||
if (!info.compCompHnd->isValueClass(clsHnd) || info.compCompHnd->getClassNumInstanceFields(clsHnd) != 1) | ||
{ | ||
return false; | ||
} | ||
|
||
CORINFO_CLASS_HANDLE* pClsHnd = &clsHnd; | ||
CORINFO_FIELD_HANDLE fldHnd = info.compCompHnd->getFieldInClass(clsHnd, 0); | ||
CorInfoType fieldType = info.compCompHnd->getFieldType(fldHnd, pClsHnd); | ||
|
||
var_types vt = JITtype2varType(fieldType); | ||
|
||
if (fieldType == CORINFO_TYPE_VALUECLASS) | ||
{ | ||
clsHnd = *pClsHnd; | ||
} | ||
else if (varTypeIsI(vt) && !varTypeIsGC(vt)) | ||
{ | ||
return true; | ||
} | ||
else | ||
{ | ||
return false; | ||
} | ||
} | ||
} | ||
#endif // TARGET_X86 | ||
|
||
//----------------------------------------------------------------------------- | ||
// getPrimitiveTypeForStruct: | ||
// Get the "primitive" type that is is used for a struct | ||
|
@@ -693,7 +743,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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will soon forget what VM used to unwrap, maybe delete an outdated reference and say something like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the body says:
Is it still true? What does this method return for a trivial struct on x86 unix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
#ifndef TARGET_X86 | ||
#ifdef UNIX_AMD64_ABI | ||
|
||
|
@@ -728,7 +778,11 @@ var_types Compiler::getArgTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
// and also examine the clsHnd to see if it is an HFA of count one | ||
useType = getPrimitiveTypeForStruct(structSize, clsHnd, isVarArg); | ||
} | ||
|
||
#else | ||
if (isTrivialPointerSizedStruct(clsHnd)) | ||
{ | ||
useType = TYP_I_IMPL; | ||
} | ||
#endif // !TARGET_X86 | ||
|
||
// Did we change this struct type into a simple "primitive" type? | ||
|
@@ -876,6 +930,8 @@ var_types Compiler::getArgTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
// | ||
// Arguments: | ||
// clsHnd - the handle for the struct type | ||
// callConv - the calling convention of the function | ||
jkoritzinsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// that returns this struct. | ||
// wbReturnStruct - An "out" argument with information about how | ||
// the struct is to be returned | ||
// structSize - the size of the struct type, | ||
|
@@ -910,9 +966,10 @@ var_types Compiler::getArgTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
// Whenever this method's return value is TYP_STRUCT it always means | ||
// that multiple registers are used to return this struct. | ||
// | ||
var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | ||
structPassingKind* wbReturnStruct /* = nullptr */, | ||
unsigned structSize /* = 0 */) | ||
var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | ||
CorInfoUnmanagedCallConv callConv, | ||
structPassingKind* wbReturnStruct /* = nullptr */, | ||
unsigned structSize /* = 0 */) | ||
{ | ||
var_types useType = TYP_UNKNOWN; | ||
structPassingKind howToReturnStruct = SPK_Unknown; // We must change this before we return | ||
|
@@ -950,10 +1007,32 @@ var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
{ | ||
// Return classification is not always size based... | ||
canReturnInRegister = structDesc.passedInRegisters; | ||
if (!canReturnInRegister) | ||
{ | ||
assert(structDesc.eightByteCount == 0); | ||
howToReturnStruct = SPK_ByReference; | ||
useType = TYP_UNKNOWN; | ||
} | ||
} | ||
|
||
#endif // UNIX_AMD64_ABI | ||
jkoritzinsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#ifdef UNIX_X86_ABI | ||
if (callConv != CORINFO_UNMANAGED_CALLCONV_MANAGED) | ||
{ | ||
canReturnInRegister = false; | ||
howToReturnStruct = SPK_ByReference; | ||
useType = TYP_UNKNOWN; | ||
} | ||
#elif defined(TARGET_WINDOWS) && !defined(TARGET_ARM) | ||
if (callConv == CORINFO_UNMANAGED_CALLCONV_THISCALL) | ||
{ | ||
canReturnInRegister = false; | ||
howToReturnStruct = SPK_ByReference; | ||
useType = TYP_UNKNOWN; | ||
} | ||
#endif | ||
|
||
// Check for cases where a small struct is returned in a register | ||
// via a primitive type. | ||
// | ||
|
@@ -1008,7 +1087,7 @@ var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
// If so, we should have already set howToReturnStruct, too. | ||
assert(howToReturnStruct != SPK_Unknown); | ||
} | ||
else // We can't replace the struct with a "primitive" type | ||
else if (canReturnInRegister) // We can't replace the struct with a "primitive" type | ||
{ | ||
// See if we can return this struct by value, possibly in multiple registers | ||
// or if we should return it using a return buffer register | ||
|
@@ -1031,24 +1110,13 @@ var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
|
||
#ifdef UNIX_AMD64_ABI | ||
|
||
// The case of (structDesc.eightByteCount == 1) should have already been handled | ||
if (structDesc.eightByteCount > 1) | ||
{ | ||
// setup wbPassType and useType indicate that this is returned by value in multiple registers | ||
howToReturnStruct = SPK_ByValue; | ||
useType = TYP_STRUCT; | ||
assert(structDesc.passedInRegisters == true); | ||
} | ||
else | ||
{ | ||
assert(structDesc.eightByteCount == 0); | ||
// Otherwise we return this struct using a return buffer | ||
// setup wbPassType and useType indicate that this is return using a return buffer register | ||
// (reference to a return buffer) | ||
howToReturnStruct = SPK_ByReference; | ||
useType = TYP_UNKNOWN; | ||
assert(structDesc.passedInRegisters == false); | ||
} | ||
// The cases of (structDesc.eightByteCount == 1) and (structDesc.eightByteCount == 0) | ||
// should have already been handled | ||
assert(structDesc.eightByteCount > 1); | ||
// setup wbPassType and useType indicate that this is returned by value in multiple registers | ||
howToReturnStruct = SPK_ByValue; | ||
useType = TYP_STRUCT; | ||
assert(structDesc.passedInRegisters == true); | ||
|
||
#elif defined(TARGET_ARM64) | ||
|
||
|
@@ -1071,8 +1139,26 @@ var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, | |
howToReturnStruct = SPK_ByReference; | ||
useType = TYP_UNKNOWN; | ||
} | ||
#elif defined(TARGET_X86) | ||
|
||
#elif defined(TARGET_ARM) || defined(TARGET_X86) | ||
// Only 8-byte structs are return in multiple registers. | ||
// We also only support multireg struct returns on x86 to match the native calling convention. | ||
// So return 8-byte structs only when the calling convention is a native calling convention. | ||
if (structSize == MAX_RET_MULTIREG_BYTES && callConv != CORINFO_UNMANAGED_CALLCONV_MANAGED) | ||
{ | ||
// setup wbPassType and useType indicate that this is return by value in multiple registers | ||
howToReturnStruct = SPK_ByValue; | ||
useType = TYP_STRUCT; | ||
} | ||
else | ||
{ | ||
// Otherwise we return this struct using a return buffer | ||
// setup wbPassType and useType indicate that this is returned using a return buffer register | ||
// (reference to a return buffer) | ||
howToReturnStruct = SPK_ByReference; | ||
useType = TYP_UNKNOWN; | ||
} | ||
#elif defined(TARGET_ARM) | ||
|
||
// Otherwise we return this struct using a return buffer | ||
// setup wbPassType and useType indicate that this is returned using a return buffer register | ||
|
@@ -1975,6 +2061,22 @@ unsigned Compiler::compGetTypeSize(CorInfoType cit, CORINFO_CLASS_HANDLE clsHnd) | |
return sigSize; | ||
} | ||
|
||
bool Compiler::compMethodIsNativeInstanceMethod(CORINFO_METHOD_INFO* mthInfo) | ||
{ | ||
return mthInfo->args.getCallConv() == CORINFO_CALLCONV_THISCALL; | ||
} | ||
|
||
CorInfoUnmanagedCallConv Compiler::compMethodInfoGetUnmanagedCallConv(CORINFO_METHOD_INFO* mthInfo) | ||
{ | ||
CorInfoCallConv callConv = mthInfo->args.getCallConv(); | ||
if (callConv == CORINFO_CALLCONV_DEFAULT || callConv == CORINFO_CALLCONV_VARARG) | ||
{ | ||
// Both the default and the varargs calling conventions represent a managed callconv. | ||
return CORINFO_UNMANAGED_CALLCONV_MANAGED; | ||
} | ||
return (CorInfoUnmanagedCallConv)callConv; | ||
} | ||
|
||
#ifdef DEBUG | ||
static bool DidComponentUnitTests = false; | ||
|
||
|
@@ -2209,7 +2311,7 @@ void Compiler::compSetProcessor() | |
#elif defined(TARGET_ARM64) | ||
info.genCPU = CPU_ARM64; | ||
#elif defined(TARGET_AMD64) | ||
info.genCPU = CPU_X64; | ||
info.genCPU = CPU_X64; | ||
#elif defined(TARGET_X86) | ||
if (jitFlags.IsSet(JitFlags::JIT_FLAG_TARGET_P4)) | ||
info.genCPU = CPU_X86_PENTIUM_4; | ||
|
@@ -5976,6 +6078,9 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr, | |
case CORINFO_CALLCONV_NATIVEVARARG: | ||
info.compIsVarArgs = true; | ||
break; | ||
case CORINFO_CALLCONV_C: | ||
case CORINFO_CALLCONV_STDCALL: | ||
case CORINFO_CALLCONV_THISCALL: | ||
case CORINFO_CALLCONV_DEFAULT: | ||
info.compIsVarArgs = false; | ||
break; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNMANAGED MANAGED looks confusing to me, why do we need this and why can't we use
CorInfoCallConv
in the Jit source and deletecompMethodInfoGetUnmanagedCallConv
?Update: I looked that it was a replacement for
CORINFO_UNMANAGED_CALLCONV_UNKNOWN
, but I recommend to try to useCorInfoCallConv
instead in the jit source code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of using
CorInfoCallConv
for two reasons:CORINFO_CALLCONV_UNMANAGED
calling convention to represent an "extensible" calling convention model that isn't directly represented by bits in metadata, so I wanted to use an enum that we could extend easily to add support for new calling conventions. TheCorInfoCallConv
enum doesn't have space to extend to add new unmanaged calling conventions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sandreenko would you prefer if I made a new enum to use instead of extending the CorInfoUnmanagedCallingConvention enum for this use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment just above this says
which is no longer true. Perhaps update that comment too? And I think it's also worth explaining what the newly added convention means, as the name sounds contradictory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments to explain the new extended usage of this enum. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can
CORINFO_UNMANAGED_CALLCONV_MANAGED
be defined asCORINFO_UNMANAGED_CALLCONV_UNKNOWN
? It would make it possible to have the definition to be local to the JIT.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone and done that.