Skip to content

[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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 52 additions & 13 deletions llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


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) {
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to avoid the magic number 3?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find one, but I'll try to reduce the number of copies of this code.

if (isa<RegisterSDNode>(N->getOperand(I)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can skip XMM / FP register operands, so I would do a GPR64 class check here before counting up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

++NumRegs;
}
if (!Subtarget->is64Bit() && NumRegs > 1)
continue;
if (NumRegs > 6)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these values of 1 and 6 are informed by the SysV C calling conventions, and are incorrect for other calling conventions. You can probably construct a test case with custom conventions that use all available GPRs for parameters and starve the register allocator out.

I think this would fix a real issue on Windows x64, which by my reading only has 7 "volatile" GPRs:
https://learn.microsoft.com/en-us/cpp/build/x64-software-conventions?view=msvc-170#x64-register-usage

  • RAX: return
  • RCX, RDX, R8, R9: 4 param
  • R10, R11: scratch

Total: 7

If there's a way to pass something in R11, maybe via the nest parameter, this might form a tail call we can't register allocate. Or maybe there's some other convention.

I think good general fixes would be to look at the CSR mask from the target calling convention and count up the available GPRs, subtract the number of GPR register operands from that total, and check that we have at least two available GPRs (for base + index).

Alternatively we could only do this fold for C calling conventions where it's known to be safe, if we convince ourselves that it's impossible to use R10 or R11 with the MS ABI convention.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, Win64 has one less. I stole my code from the X86tcret_6regs fragment:

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:

// 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:

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:

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?

continue;
}

/// Also try moving call address load from outside callseq_start to just
/// before the call to allow it to be folded.
///
Expand Down
3 changes: 1 addition & 2 deletions llvm/test/CodeGen/X86/cfguard-checks.ll
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,7 @@ entry:
; X64-LABEL: vmptr_thunk:
; X64: movq (%rcx), %rax
; X64-NEXT: movq 8(%rax), %rax
; X64-NEXT: movq __guard_dispatch_icall_fptr(%rip), %rdx
; X64-NEXT: rex64 jmpq *%rdx # TAILCALL
; X64-NEXT: rex64 jmpq *__guard_dispatch_icall_fptr(%rip) # TAILCALL
; X64-NOT: callq
}

Expand Down
26 changes: 26 additions & 0 deletions llvm/test/CodeGen/X86/fold-call-4.ll
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
}
12 changes: 12 additions & 0 deletions llvm/test/CodeGen/X86/fold-call.ll
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,15 @@ entry:
tail call void %0()
ret void
}

; Don't fold the load+call if there's inline asm in between.
; CHECK: test3
; CHECK: mov{{.*}}
; CHECK: jmp{{.*}}
define void @test3(ptr nocapture %x) {
entry:
%0 = load ptr, ptr %x
call void asm sideeffect "", ""() ; It could do anything.
tail call void %0()
ret void
}