From 6418252c4ebc07c059c4e5c8d3e12be63433f7ae Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Mon, 13 Jun 2022 22:01:47 +0300 Subject: [PATCH] Use the signature types when building ABI info (#70635) * Use signature types when building the ABI info This will allow us to let "mismatched" struct types as call arguments, which in turn is expected to simplify and de-pessimize much of the code working around the need to keep precise handles on struct nodes. Credit to @jakobbotsch for being able to make this change. * Work around the "InferOpSizeAlign" problem Handling it generally requires solving a larger problem of "eeGetArgSizeAlignment" not working well on ARM. (See also the use of "eeGetArgSizeAlignment" in "lvaInitUserArgs") --- src/coreclr/jit/codegencommon.cpp | 29 --------- src/coreclr/jit/compiler.h | 5 -- src/coreclr/jit/lower.cpp | 8 +-- src/coreclr/jit/morph.cpp | 100 +++++++++++++++--------------- 4 files changed, 54 insertions(+), 88 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index ed16b9a9fa498..044aa740a19d5 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -6581,35 +6581,6 @@ bool Compiler::IsHfa(CORINFO_CLASS_HANDLE hClass) return varTypeIsValidHfaType(GetHfaType(hClass)); } -bool Compiler::IsHfa(GenTree* tree) -{ - if (GlobalJitOptions::compFeatureHfa) - { - return IsHfa(gtGetStructHandleIfPresent(tree)); - } - else - { - return false; - } -} - -var_types Compiler::GetHfaType(GenTree* tree) -{ - if (GlobalJitOptions::compFeatureHfa) - { - return GetHfaType(gtGetStructHandleIfPresent(tree)); - } - else - { - return TYP_UNDEF; - } -} - -unsigned Compiler::GetHfaCount(GenTree* tree) -{ - return GetHfaCount(gtGetStructHandle(tree)); -} - var_types Compiler::GetHfaType(CORINFO_CLASS_HANDLE hClass) { if (GlobalJitOptions::compFeatureHfa) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index c838b9e9c7268..8df241aacce85 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1882,11 +1882,6 @@ class Compiler // bool IsHfa(CORINFO_CLASS_HANDLE hClass); - bool IsHfa(GenTree* tree); - - var_types GetHfaType(GenTree* tree); - unsigned GetHfaCount(GenTree* tree); - var_types GetHfaType(CORINFO_CLASS_HANDLE hClass); unsigned GetHfaCount(CORINFO_CLASS_HANDLE hClass); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 49dde8acd1fd4..c560f369529b0 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3814,18 +3814,18 @@ void Lowering::LowerCallStruct(GenTreeCall* call) if (GlobalJitOptions::compFeatureHfa) { - if (comp->IsHfa(call)) + if (comp->IsHfa(call->gtRetClsHnd)) { #if defined(TARGET_ARM64) - assert(comp->GetHfaCount(call) == 1); + assert(comp->GetHfaCount(call->gtRetClsHnd) == 1); #elif defined(TARGET_ARM) // ARM returns double in 2 float registers, but // `call->HasMultiRegRetVal()` count double registers. - assert(comp->GetHfaCount(call) <= 2); + assert(comp->GetHfaCount(call->gtRetClsHnd) <= 2); #else // !TARGET_ARM64 && !TARGET_ARM NYI("Unknown architecture"); #endif // !TARGET_ARM64 && !TARGET_ARM - var_types hfaType = comp->GetHfaType(call); + var_types hfaType = comp->GetHfaType(call->gtRetClsHnd); if (call->TypeIs(hfaType)) { return; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 4e65b06d9a75f..c571c48a7545f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -2233,7 +2233,12 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call argx->gtType = TYP_I_IMPL; } - // Setup any HFA information about 'argx' + // Note we must use the signature types for making ABI decisions. This is especially important for structs, + // where the "argx" node can legally have a type that is not ABI-compatible with the one in the signature. + const var_types argSigType = arg.GetSignatureType(); + const CORINFO_CLASS_HANDLE argSigClass = arg.GetSignatureClassHandle(); + + // Setup any HFA information about the argument. bool isHfaArg = false; var_types hfaType = TYP_UNDEF; unsigned hfaSlots = 0; @@ -2245,7 +2250,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call if (GlobalJitOptions::compFeatureHfa) { - hfaType = comp->GetHfaType(argx); + hfaType = comp->GetHfaType(argSigClass); isHfaArg = varTypeIsValidHfaType(hfaType); #if defined(TARGET_ARM64) @@ -2258,7 +2263,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call if (isHfaArg) { - hfaSlots = comp->GetHfaCount(argx); + hfaSlots = comp->GetHfaCount(argSigClass); // If we have a HFA struct it's possible we transition from a method that originally // only had integer types to now start having FP types. We have to communicate this @@ -2272,11 +2277,19 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call const bool isFloatHfa = (hfaType == TYP_FLOAT); #ifdef TARGET_ARM - passUsingFloatRegs = !callIsVararg && (isHfaArg || varTypeUsesFloatReg(argx)) && !comp->opts.compUseSoftFP; + passUsingFloatRegs = + !callIsVararg && (isHfaArg || varTypeUsesFloatReg(argSigType)) && !comp->opts.compUseSoftFP; bool passUsingIntRegs = passUsingFloatRegs ? false : (intArgRegNum < MAX_REG_ARG); - // We don't use the "size" return value from InferOpSizeAlign(). - comp->codeGen->InferOpSizeAlign(argx, &argAlignBytes); + // TODO-Cleanup: use "eeGetArgSizeAlignment" here. See also: https://github.com/dotnet/runtime/issues/46026. + if (varTypeIsStruct(argSigType)) + { + argAlignBytes = comp->info.compCompHnd->getClassAlignmentRequirement(argSigClass); + } + else + { + argAlignBytes = genTypeSize(argSigType); + } argAlignBytes = roundUp(argAlignBytes, TARGET_POINTER_SIZE); @@ -2303,11 +2316,11 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call #elif defined(TARGET_ARM64) assert(!callIsVararg || !isHfaArg); - passUsingFloatRegs = !callIsVararg && (isHfaArg || varTypeUsesFloatReg(argx)); + passUsingFloatRegs = !callIsVararg && (isHfaArg || varTypeUsesFloatReg(argSigType)); #elif defined(TARGET_AMD64) - passUsingFloatRegs = varTypeIsFloating(argx); + passUsingFloatRegs = varTypeIsFloating(argSigType); #elif defined(TARGET_X86) @@ -2316,7 +2329,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call #elif defined(TARGET_LOONGARCH64) assert(!callIsVararg && !isHfaArg); - passUsingFloatRegs = varTypeUsesFloatReg(argx); + passUsingFloatRegs = varTypeUsesFloatReg(argSigType); DWORD floatFieldFlags = STRUCT_NO_FLOAT_FIELD; #else @@ -2325,43 +2338,34 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call bool isBackFilled = false; unsigned nextFltArgRegNum = fltArgRegNum; // This is the next floating-point argument register number to use + bool isStructArg = varTypeIsStruct(argSigType); var_types structBaseType = TYP_STRUCT; unsigned structSize = 0; bool passStructByRef = false; - bool isStructArg; - GenTree* actualArg = argx->gtEffectiveVal(true /* Commas only */); - // // Figure out the size of the argument. This is either in number of registers, or number of // TARGET_POINTER_SIZE stack slots, or the sum of these if the argument is split between the registers and // the stack. // - isStructArg = varTypeIsStruct(argx); - // Note that we internally in the JIT can change some struct args to - // primitive args (e.g. OBJ(x) -> IND(x)). Similarly, - // the ABI type can also change from struct to primitive (e.g. a 8-byte - // struct passed in a register). So isStructArg may be false even if - // the signature type was (or is) a struct, however only in cases where - // it does not matter. - CORINFO_CLASS_HANDLE objClass = NO_CLASS_HANDLE; + if (isStructArg) { - objClass = comp->gtGetStructHandle(argx); - if (argx->TypeGet() == TYP_STRUCT) + GenTree* actualArg = argx->gtEffectiveVal(true /* Commas only */); + + // Here we look at "actualArg" to avoid calling "getClassSize". + if (actualArg->TypeGet() == TYP_STRUCT) { - // For TYP_STRUCT arguments we must have an OBJ, LCL_VAR or MKREFANY switch (actualArg->OperGet()) { case GT_OBJ: - structSize = actualArg->AsObj()->GetLayout()->GetSize(); - assert(structSize == comp->info.compCompHnd->getClassSize(objClass)); + structSize = actualArg->AsObj()->Size(); break; case GT_LCL_VAR: structSize = comp->lvaGetDesc(actualArg->AsLclVarCommon())->lvExactSize; break; case GT_MKREFANY: - structSize = comp->info.compCompHnd->getClassSize(objClass); + structSize = comp->info.compCompHnd->getClassSize(argSigClass); break; default: BADCODE("illegal argument tree: cannot determine size for ABI handling"); @@ -2370,28 +2374,29 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call } else { - structSize = genTypeSize(argx); - assert(structSize == comp->info.compCompHnd->getClassSize(objClass)); + structSize = genTypeSize(actualArg); } + + assert(structSize = comp->info.compCompHnd->getClassSize(argSigClass)); } #if defined(TARGET_AMD64) #ifdef UNIX_AMD64_ABI if (!isStructArg) { size = 1; // On AMD64, all primitives fit in a single (64-bit) 'slot' - byteSize = genTypeSize(arg.GetSignatureType()); + byteSize = genTypeSize(argSigType); } else { size = (unsigned)(roundUp(structSize, TARGET_POINTER_SIZE)) / TARGET_POINTER_SIZE; byteSize = structSize; - comp->eeGetSystemVAmd64PassStructInRegisterDescriptor(objClass, &structDesc); + comp->eeGetSystemVAmd64PassStructInRegisterDescriptor(argSigClass, &structDesc); } #else // !UNIX_AMD64_ABI size = 1; // On AMD64 Windows, all args fit in a single (64-bit) 'slot' if (!isStructArg) { - byteSize = genTypeSize(arg.GetSignatureType()); + byteSize = genTypeSize(argSigType); } #endif // UNIX_AMD64_ABI @@ -2402,9 +2407,8 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call { // HFA structs are passed by value in multiple registers. // The "size" in registers may differ the size in pointer-sized units. - CORINFO_CLASS_HANDLE structHnd = comp->gtGetStructHandle(argx); - size = comp->GetHfaCount(structHnd); - byteSize = comp->info.compCompHnd->getClassSize(structHnd); + size = hfaSlots; + byteSize = structSize; } else { @@ -2420,13 +2424,13 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call size = 1; } } - // Note that there are some additional rules for multireg structs. + // Note that there are some additional rules for multireg structs on ARM64. // (i.e they cannot be split between registers and the stack) } else { size = 1; // Otherwise, all primitive types fit in a single (64-bit) 'slot' - byteSize = genTypeSize(arg.GetSignatureType()); + byteSize = genTypeSize(argSigType); } #elif defined(TARGET_ARM) || defined(TARGET_X86) if (isStructArg) @@ -2438,8 +2442,8 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call { // The typical case. // Long/double type argument(s) will be modified as needed in Lowering. - size = genTypeStSz(argx->gtType); - byteSize = genTypeSize(arg.GetSignatureType()); + size = genTypeStSz(argSigType); + byteSize = genTypeSize(argSigType); } #else #error Unsupported or unset target architecture @@ -2451,14 +2455,14 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call assert(structSize != 0); Compiler::structPassingKind howToPassStruct; - structBaseType = comp->getArgTypeForStruct(objClass, &howToPassStruct, callIsVararg, structSize); + structBaseType = comp->getArgTypeForStruct(argSigClass, &howToPassStruct, callIsVararg, structSize); passStructByRef = (howToPassStruct == Compiler::SPK_ByReference); #if defined(TARGET_LOONGARCH64) if (!passStructByRef) { assert((howToPassStruct == Compiler::SPK_ByValue) || (howToPassStruct == Compiler::SPK_PrimitiveType)); - floatFieldFlags = comp->info.compCompHnd->getLoongArch64PassStructInRegisterFlags(objClass); + floatFieldFlags = comp->info.compCompHnd->getLoongArch64PassStructInRegisterFlags(argSigClass); passUsingFloatRegs = (floatFieldFlags & STRUCT_HAS_FLOAT_FIELDS_MASK) ? true : false; comp->compFloatingPointUsed |= passUsingFloatRegs; @@ -2469,8 +2473,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call // for "struct { float, float }", and retyping to a primitive here will cause the // multi-reg morphing to not kick in (the struct in question needs to be passed in // two FP registers). Here is just keep "structBaseType" as "TYP_STRUCT". - // TODO-LoongArch64: fix "getPrimitiveTypeForStruct" or use the ABI information in - // the arg entry instead of calling it here. + // TODO-LoongArch64: fix "getPrimitiveTypeForStruct". structBaseType = TYP_STRUCT; } @@ -2529,7 +2532,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call // Arm64 Apple has a special ABI for passing small size arguments on stack, // bytes are aligned to 1-byte, shorts to 2-byte, int/float to 4-byte, etc. // It means passing 8 1-byte arguments on stack can take as small as 8 bytes. - argAlignBytes = comp->eeGetArgSizeAlignment(arg.GetSignatureType(), isFloatHfa); + argAlignBytes = comp->eeGetArgSizeAlignment(argSigType, isFloatHfa); } #ifdef TARGET_LOONGARCH64 @@ -2541,11 +2544,11 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call bool isRegArg = false; regNumber nonStdRegNum = REG_NA; - if (isRegParamType(genActualType(argx->TypeGet())) + if (isRegParamType(genActualType(argSigType)) #ifdef UNIX_AMD64_ABI && (!isStructArg || structDesc.passedInRegisters) #elif defined(TARGET_X86) - || (isStructArg && comp->isTrivialPointerSizedStruct(objClass)) + || (isStructArg && comp->isTrivialPointerSizedStruct(argSigClass)) #endif ) { @@ -3001,12 +3004,9 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call #endif } - if (GlobalJitOptions::compFeatureHfa) + if (isHfaArg) { - if (isHfaArg) - { - arg.AbiInfo.SetHfaType(hfaType, hfaSlots); - } + arg.AbiInfo.SetHfaType(hfaType, hfaSlots); } arg.AbiInfo.SetMultiRegNums();