-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[x86] Enable indirect tail calls with more arguments #137643
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
0d00d2b
4913eea
4695fe8
9d82088
50851ca
4b99752
27716e1
b60bc94
a85c944
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -890,27 +890,50 @@ static bool isCalleeLoad(SDValue Callee, SDValue &Chain, bool HasCallSeq) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LD->getExtensionType() != ISD::NON_EXTLOAD) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If the load's outgoing chain has more than one use, we can't (currently) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // move the load since we'd most likely create a loop. TODO: Maybe it could | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // work if moveBelowOrigChain() updated *all* the chain users. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Callee.getValue(1).hasOneUse()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Now let's find the callseq_start. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (HasCallSeq && Chain.getOpcode() != ISD::CALLSEQ_START) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Chain.hasOneUse()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Chain = Chain.getOperand(0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Chain.getNumOperands()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Since we are not checking for AA here, conservatively abort if the chain | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // writes to memory. It's not safe to move the callee (a load) across a store. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isa<MemSDNode>(Chain.getNode()) && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cast<MemSDNode>(Chain.getNode())->writeMem()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (true) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Chain.getNumOperands()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Since we are not checking for AA here, conservatively abort if the chain | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // writes to memory. It's not safe to move the callee (a load) across a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // store. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isa<MemSDNode>(Chain.getNode()) && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cast<MemSDNode>(Chain.getNode())->writeMem()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Moving across inline asm is not safe: it could do anything. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Chain.getNode()->getOpcode() == ISD::INLINEASM || | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Chain.getNode()->getOpcode() == ISD::INLINEASM_BR) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Chain.getOperand(0).getNode() == Callee.getNode()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Chain.getOperand(0).getOpcode() == ISD::TokenFactor && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Chain.getOperand(0).getValue(0).hasOneUse() && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Callee.getValue(1).isOperandOf(Chain.getOperand(0).getNode()) && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Callee.getValue(1).hasOneUse()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Look past CopyToRegs. We only walk one path, so the chain mustn't branch. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Chain.getOperand(0).getOpcode() == ISD::CopyToReg && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Chain.getOperand(0).getValue(0).hasOneUse()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Chain = Chain.getOperand(0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Chain.getOperand(0).getNode() == Callee.getNode()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Chain.getOperand(0).getOpcode() == ISD::TokenFactor && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Callee.getValue(1).isOperandOf(Chain.getOperand(0).getNode()) && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Callee.getValue(1).hasOneUse()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static bool isEndbrImm64(uint64_t Imm) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1353,6 +1376,22 @@ void X86DAGToDAGISel::PreprocessISelDAG() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (N->getOpcode() == X86ISD::TC_RETURN && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (Subtarget->is64Bit() || | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !getTargetMachine().isPositionIndependent())))) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (N->getOpcode() == X86ISD::TC_RETURN) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // There needs to be enough non-callee-saved GPRs available to compute | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // the load address if folded into the tailcall. See how the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // X86tcret_6regs and X86tcret_1reg classes are used and defined. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsigned NumRegs = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (unsigned I = 3, E = N->getNumOperands(); I != E; ++I) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isa<RegisterSDNode>(N->getOperand(I))) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ++NumRegs; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Subtarget->is64Bit() && NumRegs > 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (NumRegs > 6) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def X86tcret_6regs : PatFrag<(ops node:$ptr, node:$off), | |
| (X86tcret node:$ptr, node:$off), [{ | |
| // X86tcret args: (*chain, ptr, imm, regs..., glue) | |
| unsigned NumRegs = 0; | |
| for (unsigned i = 3, e = N->getNumOperands(); i != e; ++i) | |
| if (isa<RegisterSDNode>(N->getOperand(i)) && ++NumRegs > 6) | |
| return false; | |
| return true; | |
| }]>; |
which is what the folding pattern for TCRETURNmi64 uses:
llvm-project/llvm/lib/Target/X86/X86InstrCompiler.td
Lines 1345 to 1349 in e58d227
| // Don't fold loads into X86tcret requiring more than 6 regs. | |
| // There wouldn't be enough scratch registers for base+index. | |
| def : Pat<(X86tcret_6regs (load addr:$dst), timm:$off), | |
| (TCRETURNmi64 addr:$dst, timm:$off)>, | |
| Requires<[In64BitMode, NotUseIndirectThunkCalls]>; |
So that seems wrong for Win64.
I think the source of truth here is the register class which the folded instruction actually uses, which is ptr_rc_tailcall that gets defined by X86RegisterInfo::getGPRsForTailCall:
llvm-project/llvm/lib/Target/X86/X86RegisterInfo.cpp
Lines 227 to 239 in a2c1ff1
| const TargetRegisterClass * | |
| X86RegisterInfo::getGPRsForTailCall(const MachineFunction &MF) const { | |
| const Function &F = MF.getFunction(); | |
| if (IsWin64 || (F.getCallingConv() == CallingConv::Win64)) | |
| return &X86::GR64_TCW64RegClass; | |
| else if (Is64Bit) | |
| return &X86::GR64_TCRegClass; | |
| bool hasHipeCC = (F.getCallingConv() == CallingConv::HiPE); | |
| if (hasHipeCC) | |
| return &X86::GR32RegClass; | |
| return &X86::GR32_TCRegClass; | |
| } |
That one seems to handle Win64 correctly, and also takes the calling convention into account in general.
So I think X86tcret_6regs should not hard-code 6, but check the ptr_rc_tailcall register class, and we should extract the code into a function that we can also use when moving the load.
And we should do the same for X86tcret_1reg, which is similar but has some differences:
llvm-project/llvm/lib/Target/X86/X86InstrFragments.td
Lines 686 to 699 in e58d227
| def X86tcret_1reg : PatFrag<(ops node:$ptr, node:$off), | |
| (X86tcret node:$ptr, node:$off), [{ | |
| // X86tcret args: (*chain, ptr, imm, regs..., glue) | |
| unsigned NumRegs = 1; | |
| const SDValue& BasePtr = cast<LoadSDNode>(N->getOperand(1))->getBasePtr(); | |
| if (isa<FrameIndexSDNode>(BasePtr)) | |
| NumRegs = 3; | |
| else if (BasePtr->getNumOperands() && isa<GlobalAddressSDNode>(BasePtr->getOperand(0))) | |
| NumRegs = 3; | |
| for (unsigned i = 3, e = N->getNumOperands(); i != e; ++i) | |
| if (isa<RegisterSDNode>(N->getOperand(i)) && ( NumRegs-- == 0)) | |
| return false; | |
| return true; | |
| }]>; |
It's checking whether the load uses a frame slot or a global, in which case it figures that doesn't use up any extra registers. I'm not 100% convinced that's true for the global case? And shouldn't we do the same check in X86tcret_6regs?
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefix=LIN | ||
| ; RUN: llc < %s -mtriple=x86_64-pc-windows-msvc | FileCheck %s --check-prefix=WIN | ||
|
|
||
| ; The callee address computation should get folded into the call. | ||
| ; CHECK-LABEL: f: | ||
| ; CHECK-NOT: mov | ||
| ; LIN: jmpq *(%rdi,%rsi,8) | ||
| ; WIN: rex64 jmpq *(%rcx,%rdx,8) | ||
| define void @f(ptr %table, i64 %idx, i64 %aux1, i64 %aux2, i64 %aux3) { | ||
| entry: | ||
| %arrayidx = getelementptr inbounds ptr, ptr %table, i64 %idx | ||
| %funcptr = load ptr, ptr %arrayidx, align 8 | ||
| tail call void %funcptr(ptr %table, i64 %idx, i64 %aux1, i64 %aux2, i64 %aux3) | ||
| ret void | ||
| } | ||
|
|
||
| ; Check that we don't assert here. On Win64 this has a TokenFactor with | ||
| ; multiple uses, which we can't currently fold. | ||
| define void @thunk(ptr %this, ...) { | ||
| entry: | ||
| %vtable = load ptr, ptr %this, align 8 | ||
| %vfn = getelementptr inbounds nuw i8, ptr %vtable, i64 8 | ||
| %0 = load ptr, ptr %vfn, align 8 | ||
| musttail call void (ptr, ...) %0(ptr %this, ...) | ||
| ret void | ||
| } |
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.
Please just allow specific nodes, and forbid anything unknown. Trying to list out every possible relevant node is guaranteed to fall out of date at some point, even if you manage to come up with a complete 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.
Done.