-
Notifications
You must be signed in to change notification settings - Fork 18.1k
[X86][Windows] Return fp128 on the stack
#204887
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
Changes from 9 commits
6306970
70e8db9
6f909ce
404e211
c51d0c8
1d4dea5
4cb0cac
8e66e5a
9fd204f
484e270
c840be9
2a62e40
6e62b7c
d64b8f3
bbedb30
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 |
|---|---|---|
|
|
@@ -1421,12 +1421,12 @@ class WinX86_64ABIInfo : public ABIInfo { | |
| ABIArgInfo classifyArgForArm64ECVarArg(QualType Ty) const override { | ||
| unsigned FreeSSERegs = 0; | ||
| return classify(Ty, FreeSSERegs, /*IsReturnType=*/false, | ||
| /*IsVectorCall=*/false, /*IsRegCall=*/false); | ||
| llvm::CallingConv::C); | ||
| } | ||
|
|
||
| private: | ||
| ABIArgInfo classify(QualType Ty, unsigned &FreeSSERegs, bool IsReturnType, | ||
| bool IsVectorCall, bool IsRegCall) const; | ||
| unsigned CC) const; | ||
| ABIArgInfo reclassifyHvaArgForVectorCall(QualType Ty, unsigned &FreeSSERegs, | ||
| const ABIArgInfo ¤t) const; | ||
|
|
||
|
|
@@ -3338,8 +3338,9 @@ ABIArgInfo WinX86_64ABIInfo::reclassifyHvaArgForVectorCall( | |
| } | ||
|
|
||
| ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, | ||
| bool IsReturnType, bool IsVectorCall, | ||
| bool IsRegCall) const { | ||
| bool IsReturnType, unsigned CC) const { | ||
| bool IsVectorCall = CC == llvm::CallingConv::X86_VectorCall; | ||
| bool IsRegCall = CC == llvm::CallingConv::X86_RegCall; | ||
|
|
||
| if (Ty->isVoidType()) | ||
| return ABIArgInfo::getIgnore(); | ||
|
|
@@ -3437,8 +3438,6 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, | |
| case BuiltinType::Int128: | ||
| case BuiltinType::UInt128: | ||
| case BuiltinType::Float128: | ||
| // 128-bit float and integer types share the same ABI. | ||
|
|
||
| // If it's a parameter type, the normal ABI rule is that arguments larger | ||
| // than 8 bytes are passed indirectly. GCC follows it. We follow it too, | ||
| // even though it isn't particularly efficient. | ||
|
|
@@ -3449,10 +3448,31 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, | |
|
|
||
| // Mingw64 GCC returns i128 in XMM0. Coerce to v2i64 to handle that. | ||
| // Clang matches them for compatibility. | ||
| // NOTE: GCC actually returns f128 indirectly but will hopefully change. | ||
| // See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054#c8. | ||
| return ABIArgInfo::getDirect(llvm::FixedVectorType::get( | ||
| llvm::Type::getInt64Ty(getVMContext()), 2)); | ||
| if (BT->getKind() == BuiltinType::Int128 || | ||
| BT->getKind() == BuiltinType::UInt128) | ||
| return ABIArgInfo::getDirect(llvm::FixedVectorType::get( | ||
| llvm::Type::getInt64Ty(getVMContext()), 2)); | ||
|
|
||
| // Mingw64 GCC returns f128 via sret, and Clang matches that for | ||
| // compatibility. This mirrors the X86 backend's CanLowerReturn logic. | ||
| if (BT->getKind() == BuiltinType::Float128) { | ||
| auto IsWin64F128StackCC = [this](unsigned CC) -> bool { | ||
| switch (CC) { | ||
| case llvm::CallingConv::Win64: | ||
| return true; | ||
| case llvm::CallingConv::C: | ||
| return getTarget().getTriple().isOSWindows() || | ||
|
Contributor
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 added
Contributor
Author
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. Neat, I'm using that now. |
||
| getTarget().getTriple().isUEFI(); | ||
| default: | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| if (IsWin64F128StackCC(CC)) | ||
| return getNaturalAlignIndirect( | ||
| Ty, getDataLayout().getAllocaAddrSpace(), /*ByVal=*/false); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
|
|
@@ -3498,8 +3518,7 @@ void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const { | |
| } | ||
|
|
||
| if (!getCXXABI().classifyReturnType(FI)) | ||
| FI.getReturnInfo() = classify(FI.getReturnType(), FreeSSERegs, true, | ||
| IsVectorCall, IsRegCall); | ||
| FI.getReturnInfo() = classify(FI.getReturnType(), FreeSSERegs, true, CC); | ||
|
|
||
| if (IsVectorCall) { | ||
| // We can use up to 6 SSE register parameters with vectorcall. | ||
|
|
@@ -3517,8 +3536,7 @@ void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const { | |
| // registers are left. | ||
| unsigned *MaybeFreeSSERegs = | ||
| (IsVectorCall && ArgNum >= 6) ? &ZeroSSERegs : &FreeSSERegs; | ||
| I.info = | ||
| classify(I.type, *MaybeFreeSSERegs, false, IsVectorCall, IsRegCall); | ||
| I.info = classify(I.type, *MaybeFreeSSERegs, false, CC); | ||
| ++ArgNum; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,21 @@ | |
| // __float128 is unsupported on MSVC | ||
|
|
||
| __float128 fp128_ret(void) { return 0; } | ||
| // CHECK-GNU64: define dso_local <2 x i64> @fp128_ret() | ||
| // CHECK-GNU64: define dso_local void @fp128_ret(ptr dead_on_unwind noalias writable sret(fp128) align 16 %agg.result) | ||
|
|
||
| __float128 fp128_args(__float128 a, __float128 b) { return a * b; } | ||
| // CHECK-GNU64: define dso_local <2 x i64> @fp128_args(ptr noundef dead_on_return %0, ptr noundef dead_on_return %1) | ||
| // CHECK-GNU64: define dso_local void @fp128_args(ptr dead_on_unwind noalias writable sret(fp128) align 16 %agg.result, ptr noundef dead_on_return %0, ptr noundef dead_on_return %1) | ||
|
|
||
| __float128 __attribute__((vectorcall)) fp128_ret_vectorcall(void) { return 0; } | ||
| // CHECK-GNU64: define dso_local x86_vectorcallcc fp128 @"\01fp128_ret_vectorcall@@0"() | ||
|
Comment on lines
+11
to
+12
Contributor
Author
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 returns |
||
|
|
||
| __float128 __attribute__((regcall)) fp128_ret_regcall(void) { return 0; } | ||
| // CHECK-GNU64: define dso_local x86_regcallcc fp128 @__regcall3__fp128_ret_regcall() | ||
|
|
||
| __float128 fp128_callee(void); | ||
| __float128 fp128_musttail(void) { [[clang::musttail]] return fp128_callee(); } | ||
| // CHECK-GNU64: define dso_local void @fp128_musttail(ptr dead_on_unwind noalias writable sret(fp128) align 16 %agg.result) | ||
| // CHECK-GNU64: musttail call void @fp128_callee(ptr dead_on_unwind writable sret(fp128) align 16 %agg.result) | ||
|
|
||
| void fp128_vararg(int a, ...) { | ||
| // CHECK-GNU64-LABEL: define dso_local void @fp128_vararg | ||
|
|
||
|
Contributor
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. Just thinking as somebody who has chased ABI code around - might be worth a note in |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -670,6 +670,43 @@ bool X86TargetLowering::CanLowerReturn( | |
| CallingConv::ID CallConv, MachineFunction &MF, bool isVarArg, | ||
| const SmallVectorImpl<ISD::OutputArg> &Outs, LLVMContext &Context, | ||
| const Type *RetTy) const { | ||
| // Mingw64 GCC returns f128 via sret, and LLVM matches it for compatibility. | ||
| // This logic exists for libcalls, a frontend should explicitly use sret | ||
| // rather than rely on the sret demotion here. | ||
| // | ||
| // Using sret is a reasonable implementation of the Windows x64 calling | ||
| // convention: | ||
| // | ||
| // https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170#return-values | ||
| // | ||
| // > Otherwise, the caller must allocate memory for the return value and pass | ||
| // > a pointer to it as the first argument. | ||
| // | ||
| // Although it is not the only reasonable interpretation: | ||
| // | ||
| // > Nonscalar types including floats, doubles, and vector types such as | ||
| // > __m128, __m128i, __m128d are returned in XMM0. | ||
| // | ||
| // For now, we prefer compatibility with GCC. If official guidelines are ever | ||
| // published, this can be revisited. | ||
| // | ||
| // Return false, which will perform sret demotion. | ||
| auto IsWin64F128StackCC = [this](CallingConv::ID CC) -> bool { | ||
| switch (CC) { | ||
| case CallingConv::Win64: | ||
| return true; | ||
| case CallingConv::C: | ||
| return Subtarget.isTargetWin64() || Subtarget.isTargetUEFI64(); | ||
|
Contributor
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 added a predicate for this just now |
||
| default: | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| if (IsWin64F128StackCC(CallConv) && | ||
| llvm::any_of( | ||
| Outs, [](const ISD::OutputArg &Out) { return Out.VT == MVT::f128; })) | ||
| return false; | ||
|
|
||
| SmallVector<CCValAssign, 16> RVLocs; | ||
| CCState CCInfo(CallConv, isVarArg, MF, RVLocs, Context); | ||
| return CCInfo.CheckReturn(Outs, RetCC_X86); | ||
|
|
||
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.
VectorCallis, from what I can find, clang-specific and so I don't think we need to be compatible with anything for it. Leaving it as before is easier.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.
No, it's this: https://learn.microsoft.com/en-us/cpp/cpp/vectorcall?view=msvc-170
GCC might wish to support it one day, I would consider it to be part of the platform C ABI, even if it does have some quirks that require two iterations over the argument list.
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.
Ah, sorry yeah I meant that clang is the only compiler that needs to decide what
f128does for that calling convention. And given the name, here it kind of would make sense for GCC to pick passing/returning via vector registers?In any case, anything we do here is arbitrary, and switching it over is kind of ugly code-wise.
Uh oh!
There was an error while loading. Please reload this page.
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 haven't looked at what this code does but my assumption is yes, for vectorcall f128 should ideally be returned in xmm0 (like what LLVM does for the C convention today) and passed that way too (unlike the C convention)
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.
We currently pass
f128via XMM registers (below xmm0 contains %a, rdx contains %p):The current code also returns via xmm0 for vectorcall.
So yeah, we can discuss this but it seems quite reasonable? And it can be changed later if there is something we'd want to be compatible with that makes a different choice.
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 think I agree, if MSVC ever adds f128 support, I strongly suspect they'll choose to return in XMM0, so this is probably the right set of conventions.