[ARM/X86] Standardize the isEligibleForTailCallOptimization prototypes #90688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pass in CallLoweringInfo (CLI) instead of passing in the various fields directly. Also pass in CCState (CCInfo), which is computed in both the caller and the callee for a minor efficiency saving. There may also be a small correctness improvement for sibcalls with vectorcall, which has an odd way of recomputing argument locations.
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-arm Author: Reid Kleckner (rnk) ChangesPass in CallLoweringInfo (CLI) instead of passing in the various fields directly. Also pass in CCState (CCInfo), which is computed in both the caller and the callee for a minor efficiency saving. There may also be a small correctness improvement for sibcalls with vectorcall, which has an odd way of recomputing argument locations. This is a step towards improving the handling of musttail on armv7, which we have numerous issues filed about in our tracker. I took inspiration for this from the RISCV tail call eligibility check, which uses a similar prototype. Full diff: https://github.com/llvm/llvm-project/pull/90688.diff 4 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index f67a68acbf23d4..b97225cfb0c0d9 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -2366,6 +2366,12 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
bool PreferIndirect = false;
bool GuardWithBTI = false;
+ // Analyze operands of the call, assigning locations to each operand.
+ SmallVector<CCValAssign, 16> ArgLocs;
+ CCState CCInfo(CallConv, isVarArg, DAG.getMachineFunction(), ArgLocs,
+ *DAG.getContext());
+ CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CallConv, isVarArg));
+
// Lower 'returns_twice' calls to a pseudo-instruction.
if (CLI.CB && CLI.CB->getAttributes().hasFnAttr(Attribute::ReturnsTwice) &&
!Subtarget->noBTIAtReturnTwice())
@@ -2401,10 +2407,7 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
}
if (isTailCall) {
// Check if it's really possible to do a tail call.
- isTailCall = IsEligibleForTailCallOptimization(
- Callee, CallConv, isVarArg, isStructRet,
- MF.getFunction().hasStructRetAttr(), Outs, OutVals, Ins, DAG,
- PreferIndirect);
+ isTailCall = IsEligibleForTailCallOptimization(CLI, CCInfo, ArgLocs, PreferIndirect);
if (isTailCall && !getTargetMachine().Options.GuaranteedTailCallOpt &&
CallConv != CallingConv::Tail && CallConv != CallingConv::SwiftTail)
@@ -2419,11 +2422,6 @@ ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
if (!isTailCall && CLI.CB && CLI.CB->isMustTailCall())
report_fatal_error("failed to perform tail call elimination on a call "
"site marked musttail");
- // Analyze operands of the call, assigning locations to each operand.
- SmallVector<CCValAssign, 16> ArgLocs;
- CCState CCInfo(CallConv, isVarArg, DAG.getMachineFunction(), ArgLocs,
- *DAG.getContext());
- CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CallConv, isVarArg));
// Get a count of how many bytes are to be pushed on the stack.
unsigned NumBytes = CCInfo.getStackSize();
@@ -2987,12 +2985,15 @@ bool MatchingStackOffset(SDValue Arg, unsigned Offset, ISD::ArgFlagsTy Flags,
/// for tail call optimization. Targets which want to do tail call
/// optimization should implement this function.
bool ARMTargetLowering::IsEligibleForTailCallOptimization(
- SDValue Callee, CallingConv::ID CalleeCC, bool isVarArg,
- bool isCalleeStructRet, bool isCallerStructRet,
- const SmallVectorImpl<ISD::OutputArg> &Outs,
- const SmallVectorImpl<SDValue> &OutVals,
- const SmallVectorImpl<ISD::InputArg> &Ins, SelectionDAG &DAG,
- const bool isIndirect) const {
+ TargetLowering::CallLoweringInfo &CLI, CCState &CCInfo,
+ SmallVectorImpl<CCValAssign> &ArgLocs, const bool isIndirect) const {
+ CallingConv::ID CalleeCC = CLI.CallConv;
+ SDValue Callee = CLI.Callee;
+ bool isVarArg = CLI.IsVarArg;
+ const SmallVector<ISD::OutputArg, 32> &Outs = CLI.Outs;
+ const SmallVector<SDValue, 32> &OutVals = CLI.OutVals;
+ const SmallVector<ISD::InputArg, 32> &Ins = CLI.Ins;
+ const SelectionDAG &DAG = CLI.DAG;
MachineFunction &MF = DAG.getMachineFunction();
const Function &CallerF = MF.getFunction();
CallingConv::ID CallerCC = CallerF.getCallingConv();
@@ -3028,6 +3029,8 @@ bool ARMTargetLowering::IsEligibleForTailCallOptimization(
// Also avoid sibcall optimization if either caller or callee uses struct
// return semantics.
+ bool isCalleeStructRet = (Outs.empty()) ? false : Outs[0].Flags.isSRet();
+ bool isCallerStructRet = MF.getFunction().hasStructRetAttr();
if (isCalleeStructRet || isCallerStructRet)
return false;
@@ -3073,11 +3076,6 @@ bool ARMTargetLowering::IsEligibleForTailCallOptimization(
// If the callee takes no arguments then go on to check the results of the
// call.
if (!Outs.empty()) {
- // Check if stack adjustment is needed. For now, do not do this if any
- // argument is passed on the stack.
- SmallVector<CCValAssign, 16> ArgLocs;
- CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
- CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CalleeCC, isVarArg));
if (CCInfo.getStackSize()) {
// Check if the arguments are already laid out in the right way as
// the caller's fixed stack objects.
diff --git a/llvm/lib/Target/ARM/ARMISelLowering.h b/llvm/lib/Target/ARM/ARMISelLowering.h
index 26ef295e3d3fc3..ed4df7edd16e6c 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.h
+++ b/llvm/lib/Target/ARM/ARMISelLowering.h
@@ -943,12 +943,8 @@ class VectorType;
/// for tail call optimization. Targets which want to do tail call
/// optimization should implement this function.
bool IsEligibleForTailCallOptimization(
- SDValue Callee, CallingConv::ID CalleeCC, bool isVarArg,
- bool isCalleeStructRet, bool isCallerStructRet,
- const SmallVectorImpl<ISD::OutputArg> &Outs,
- const SmallVectorImpl<SDValue> &OutVals,
- const SmallVectorImpl<ISD::InputArg> &Ins, SelectionDAG &DAG,
- const bool isIndirect) const;
+ TargetLowering::CallLoweringInfo &CLI, CCState &CCInfo,
+ SmallVectorImpl<CCValAssign> &ArgLocs, const bool isIndirect) const;
bool CanLowerReturn(CallingConv::ID CallConv,
MachineFunction &MF, bool isVarArg,
diff --git a/llvm/lib/Target/X86/X86ISelLowering.h b/llvm/lib/Target/X86/X86ISelLowering.h
index e348ba6e8ac085..ade54f73bff099 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.h
+++ b/llvm/lib/Target/X86/X86ISelLowering.h
@@ -1632,10 +1632,8 @@ namespace llvm {
/// Check whether the call is eligible for tail call optimization. Targets
/// that want to do tail call optimization should implement this function.
bool IsEligibleForTailCallOptimization(
- SDValue Callee, CallingConv::ID CalleeCC, bool IsCalleeStackStructRet,
- bool isVarArg, Type *RetTy, const SmallVectorImpl<ISD::OutputArg> &Outs,
- const SmallVectorImpl<SDValue> &OutVals,
- const SmallVectorImpl<ISD::InputArg> &Ins, SelectionDAG &DAG) const;
+ TargetLowering::CallLoweringInfo &CLI, CCState &CCInfo,
+ SmallVectorImpl<CCValAssign> &ArgLocs, bool IsCalleePopSRet) const;
SDValue EmitTailCallLoadRetAddr(SelectionDAG &DAG, SDValue &OutRetAddr,
SDValue Chain, bool IsTailCall,
bool Is64Bit, int FPDiff,
diff --git a/llvm/lib/Target/X86/X86ISelLoweringCall.cpp b/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
index 1f76f74510335c..49b14b2033d369 100644
--- a/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
+++ b/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
@@ -2021,6 +2021,22 @@ X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
if (CallConv == CallingConv::X86_INTR)
report_fatal_error("X86 interrupts may not be called directly");
+ // Analyze operands of the call, assigning locations to each operand.
+ SmallVector<CCValAssign, 16> ArgLocs;
+ CCState CCInfo(CallConv, isVarArg, MF, ArgLocs, *DAG.getContext());
+
+ // Allocate shadow area for Win64.
+ if (IsWin64)
+ CCInfo.AllocateStack(32, Align(8));
+
+ CCInfo.AnalyzeArguments(Outs, CC_X86);
+
+ // In vectorcall calling convention a second pass is required for the HVA
+ // types.
+ if (CallingConv::X86_VectorCall == CallConv) {
+ CCInfo.AnalyzeArgumentsSecondPass(Outs, CC_X86);
+ }
+
bool IsMustTail = CLI.CB && CLI.CB->isMustTailCall();
if (Subtarget.isPICStyleGOT() && !IsGuaranteeTCO && !IsMustTail) {
// If we are using a GOT, disable tail calls to external symbols with
@@ -2036,9 +2052,8 @@ X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
if (isTailCall && !IsMustTail) {
// Check if it's really possible to do a tail call.
- isTailCall = IsEligibleForTailCallOptimization(
- Callee, CallConv, IsCalleePopSRet, isVarArg, CLI.RetTy, Outs, OutVals,
- Ins, DAG);
+ isTailCall = IsEligibleForTailCallOptimization(CLI, CCInfo, ArgLocs,
+ IsCalleePopSRet);
// Sibcalls are automatically detected tailcalls which do not require
// ABI changes.
@@ -2056,22 +2071,6 @@ X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
assert(!(isVarArg && canGuaranteeTCO(CallConv)) &&
"Var args not supported with calling convention fastcc, ghc or hipe");
- // Analyze operands of the call, assigning locations to each operand.
- SmallVector<CCValAssign, 16> ArgLocs;
- CCState CCInfo(CallConv, isVarArg, MF, ArgLocs, *DAG.getContext());
-
- // Allocate shadow area for Win64.
- if (IsWin64)
- CCInfo.AllocateStack(32, Align(8));
-
- CCInfo.AnalyzeArguments(Outs, CC_X86);
-
- // In vectorcall calling convention a second pass is required for the HVA
- // types.
- if (CallingConv::X86_VectorCall == CallConv) {
- CCInfo.AnalyzeArgumentsSecondPass(Outs, CC_X86);
- }
-
// Get a count of how many bytes are to be pushed on the stack.
unsigned NumBytes = CCInfo.getAlignedCallFrameSize();
if (IsSibcall)
@@ -2723,11 +2722,19 @@ bool MatchingStackOffset(SDValue Arg, unsigned Offset, ISD::ArgFlagsTy Flags,
/// Check whether the call is eligible for tail call optimization. Targets
/// that want to do tail call optimization should implement this function.
+/// Note that musttail calls are not checked for eligibility, so the backend
+/// must support forwarding arguments of any type.
bool X86TargetLowering::IsEligibleForTailCallOptimization(
- SDValue Callee, CallingConv::ID CalleeCC, bool IsCalleePopSRet,
- bool isVarArg, Type *RetTy, const SmallVectorImpl<ISD::OutputArg> &Outs,
- const SmallVectorImpl<SDValue> &OutVals,
- const SmallVectorImpl<ISD::InputArg> &Ins, SelectionDAG &DAG) const {
+ TargetLowering::CallLoweringInfo &CLI, CCState &CCInfo,
+ SmallVectorImpl<CCValAssign> &ArgLocs, bool IsCalleePopSRet) const {
+ SelectionDAG &DAG = CLI.DAG;
+ SmallVectorImpl<ISD::OutputArg> &Outs = CLI.Outs;
+ SmallVectorImpl<SDValue> &OutVals = CLI.OutVals;
+ SmallVectorImpl<ISD::InputArg> &Ins = CLI.Ins;
+ SDValue Callee = CLI.Callee;
+ CallingConv::ID CalleeCC = CLI.CallConv;
+ bool isVarArg = CLI.IsVarArg;
+
if (!mayTailCallThisCC(CalleeCC))
return false;
@@ -2738,7 +2745,7 @@ bool X86TargetLowering::IsEligibleForTailCallOptimization(
// If the function return type is x86_fp80 and the callee return type is not,
// then the FP_EXTEND of the call result is not a nop. It's not safe to
// perform a tailcall optimization here.
- if (CallerF.getReturnType()->isX86_FP80Ty() && !RetTy->isX86_FP80Ty())
+ if (CallerF.getReturnType()->isX86_FP80Ty() && !CLI.RetTy->isX86_FP80Ty())
return false;
CallingConv::ID CallerCC = CallerF.getCallingConv();
@@ -2791,9 +2798,6 @@ bool X86TargetLowering::IsEligibleForTailCallOptimization(
if (IsCalleeWin64 || IsCallerWin64)
return false;
- SmallVector<CCValAssign, 16> ArgLocs;
- CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
- CCInfo.AnalyzeCallOperands(Outs, CC_X86);
for (const auto &VA : ArgLocs)
if (!VA.isRegLoc())
return false;
@@ -2811,8 +2815,8 @@ bool X86TargetLowering::IsEligibleForTailCallOptimization(
}
if (Unused) {
SmallVector<CCValAssign, 16> RVLocs;
- CCState CCInfo(CalleeCC, false, MF, RVLocs, C);
- CCInfo.AnalyzeCallResult(Ins, RetCC_X86);
+ CCState RVCCInfo(CalleeCC, false, MF, RVLocs, C);
+ RVCCInfo.AnalyzeCallResult(Ins, RetCC_X86);
for (const auto &VA : RVLocs) {
if (VA.getLocReg() == X86::FP0 || VA.getLocReg() == X86::FP1)
return false;
@@ -2832,24 +2836,12 @@ bool X86TargetLowering::IsEligibleForTailCallOptimization(
return false;
}
- unsigned StackArgsSize = 0;
+ unsigned StackArgsSize = CCInfo.getStackSize();
// If the callee takes no arguments then go on to check the results of the
// call.
if (!Outs.empty()) {
- // Check if stack adjustment is needed. For now, do not do this if any
- // argument is passed on the stack.
- SmallVector<CCValAssign, 16> ArgLocs;
- CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
-
- // Allocate shadow area for Win64
- if (IsCalleeWin64)
- CCInfo.AllocateStack(32, Align(8));
-
- CCInfo.AnalyzeCallOperands(Outs, CC_X86);
- StackArgsSize = CCInfo.getStackSize();
-
- if (CCInfo.getStackSize()) {
+ if (StackArgsSize > 0) {
// Check if the arguments are already laid out in the right way as
// the caller's fixed stack objects.
MachineFrameInfo &MFI = MF.getFrameInfo();
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| CallingConv::ID CalleeCC = CLI.CallConv; | ||
| SDValue Callee = CLI.Callee; | ||
| bool isVarArg = CLI.IsVarArg; | ||
| const SmallVector<ISD::OutputArg, 32> &Outs = CLI.Outs; |
| Callee, CallConv, isVarArg, isStructRet, | ||
| MF.getFunction().hasStructRetAttr(), Outs, OutVals, Ins, DAG, | ||
| PreferIndirect); | ||
| isTailCall = IsEligibleForTailCallOptimization(CLI, CCInfo, ArgLocs, PreferIndirect); |
| @@ -2723,11 +2722,19 @@ bool MatchingStackOffset(SDValue Arg, unsigned Offset, ISD::ArgFlagsTy Flags, | |||
|
|
|||
| /// Check whether the call is eligible for tail call optimization. Targets | |||
| /// that want to do tail call optimization should implement this function. | |||
| /// Note that musttail calls are not checked for eligibility, so the backend | |||
| /// must support forwarding arguments of any type. | |||
There was a problem hiding this comment.
This doesn't really get the relevant point across... I would say:
This function is also used to reject musttail calls the backend does not know how to emit. musttail calls should be supported for all combinations of argument/return type.
There was a problem hiding this comment.
What you said is true for the ARM backend, but this is the X86 backend, where the opposite is true, so I think this is correct as written, but it could be phrased even more strongly. This was a point of disagreement in the past, how to approach musttail call eligibility checks.
The ARM backend is extremely conservative, resulting in rejecting tail calls we can probably support (sret forwarding). The X86 backend aggressively assumes it can support everything, because it already does in order to support GuaranteedTCO (not supported by ARM). In practice, this does result in miscompilations (byval forwarding, #46402), so there is a tradeoff between optimistic and conservative.
There was a problem hiding this comment.
Oh, I see... in that case, maybe just say that the x86 backend specifically doesn't call this for musttail calls.
(I generally support reporting errors over miscompiling; there are probably arguments either way for structuring this specific part of the code, though.)
aeubanks
reviewed
May 2, 2024
| /// optimization should implement this function. | ||
| /// optimization should implement this function. Note that this function also | ||
| /// processes musttail calls, in which case any case where this function returns | ||
| /// true for valid musttail call results in a fatal backend error. |
aeubanks
approved these changes
May 2, 2024
|
Thanks for the reviews! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pass in CallLoweringInfo (CLI) instead of passing in the various fields directly. Also pass in CCState (CCInfo), which is computed in both the caller and the callee for a minor efficiency saving. There may also be a small correctness improvement for sibcalls with vectorcall, which has an odd way of recomputing argument locations.
This is a step towards improving the handling of musttail on armv7, which we have numerous issues filed about in our tracker.
I took inspiration for this from the RISCV tail call eligibility check, which uses a similar prototype.