X86: Use a callee save register for the swiftself parameter.

[MatzeB]

Summary

It is very likely that the swiftself parameter is alive throughout most
functions function so putting it into a callee save register should
avoid spills for the callers with only a minimum amount of extra spills
in the callees.

Currently the generated code is correct but unnecessarily spills and
reloads arguments passed in callee save registers, I will address this
in upcoming patches.

This also adds a missing check that for tail calls the preserved value
of the caller must be the same as the callees parameter.

[mgrang]

lib/Target/X86/X86CallingConv.td

297

Type: Should be SwiftSelf instead of SwiftSwlf.

297

Typo*

test/CodeGen/X86/swiftself.ll

2

We can get rid of --check-prefix=CHECK

[MatzeB]

Fix typo, remove unnecessary --check-prefix argument.

[qcolombet]

lib/Target/X86/X86FrameLowering.cpp

1889

Perhaps add that this is conservatively correct but maybe not optimal.

test/CodeGen/X86/swiftself.ll

2

Could you re-add the i386 tests?

[MatzeB]

Fix "OPT:" check lines in test.

[MatzeB]

test/CodeGen/X86/swiftself.ll

2

Is it worth testing a calling a convention that does not behave any different for SwiftSelf? We're not testing fastcall or GHC either...

[MatzeB]

Add a comment that omtting the kill flag is conservatively correct.

[MatzeB]

Add check that for tail calls the preserved value of the caller is the same as the callees parameter.

[MatzeB]

Move to R13 to have swiftself/swifterror numbers next to each other...

[MatzeB]

Replace sneaky tabs with whitespace.

[qcolombet]

Hi Matthias,

LGTM with nits.

If you find a way to make the code that you added available for all targets that would be great. I am guessing all the targets will need to do the same checks.

No need to address that in that patch.

Cheers,
-Quentin

lib/Target/X86/X86ISelLowering.cpp

3859 ↗

(On Diff #53358)

Invert the condition and continue (LLVM coding style).

3861 ↗

(On Diff #53358)

Maybe add an example that uses the variables defined here.
E.g.,
foo:
ArgReg = COPY Reg ; Foo needs to preserve Reg.
Reg = def ; Make sure def is a copy of ArgReg.
call tailcallCandidate

3865 ↗

(On Diff #53358)

tabs :)

3869 ↗

(On Diff #53358)

Is there a way to have this check somewhere in the generic part? Maybe even just available as a utility function somewhere?

I am guessing that all targets would need to do something similar if they pass argument in a callee saved register.

[MatzeB]

Thanks for the review.

lib/Target/X86/X86ISelLowering.cpp

3869 ↗

(On Diff #53491)

Yes should be possible to factor this out into TargetLower, I will do an NFC refactoring commit afterwards.

lib/Target/X86/X86CallingConv.td

def CC_X86_64_C : CallingConv<[
 
   // Promote i1/i8/i16 arguments to i32.
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
 
   // The 'nest' parameter, if any, is passed in R10.
   CCIfNest<CCIfSubtarget<"isTarget64BitILP32()", CCAssignToReg<[R10D]>>>,
   CCIfNest<CCAssignToReg<[R10]>>,
 
-  // A SwiftSelf is passed in R10.
-  CCIfSwiftSelf<CCIfType<[i64], CCAssignToReg<[R10]>>>,
+  // Pass SwiftSwlf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i64], CCAssignToReg<[R12]>>>,
 
   // The first 6 integer arguments are passed in integer registers.
   CCIfType<[i32], CCAssignToReg<[EDI, ESI, EDX, ECX, R8D, R9D]>>,
   CCIfType<[i64], CCAssignToReg<[RDI, RSI, RDX, RCX, R8 , R9 ]>>,
 
   // The first 8 MMX vector arguments are passed in XMM registers on Darwin.
   CCIfType<[x86mmx],
             CCIfSubtarget<"isTargetDarwin()",

lib/Target/X86/X86FrameLowering.cpp

bool X86FrameLowering::spillCalleeSavedRegisters(
   DebugLoc DL = MBB.findDebugLoc(MI);
 
   // Don't save CSRs in 32-bit EH funclets. The caller saves EBX, EBP, ESI, EDI
   // for us, and there are no XMM CSRs on Win32.
   if (MBB.isEHFuncletEntry() && STI.is32Bit() && STI.isOSWindows())
     return true;
 
   // Push GPRs. It increases frame size.
+  const MachineFunction &MF = *MBB.getParent();
   unsigned Opc = STI.is64Bit() ? X86::PUSH64r : X86::PUSH32r;
   for (unsigned i = CSI.size(); i != 0; --i) {
     unsigned Reg = CSI[i - 1].getReg();
 
     if (!X86::GR64RegClass.contains(Reg) && !X86::GR32RegClass.contains(Reg))
       continue;
-    // Add the callee-saved register as live-in. It's killed at the spill.
+
+    bool isLiveIn = MF.getRegInfo().isLiveIn(Reg);
+    if (!isLiveIn)
       MBB.addLiveIn(Reg);
 
-    BuildMI(MBB, MI, DL, TII.get(Opc)).addReg(Reg, RegState::Kill)
+    // Do not set a kill flag on values that are also marked as live-in. This
+    // happens with the @llvm-returnaddress intrinsic and with arguments
+    // passed in callee saved registers.
+    bool isKill = !isLiveIn;
+
+    BuildMI(MBB, MI, DL, TII.get(Opc)).addReg(Reg, getKillRegState(isKill))
       .setMIFlag(MachineInstr::FrameSetup);
   }
 
   // Make XMM regs spilled. X86 does not have ability of push/pop XMM.
   // It can be done by spilling XMMs to stack frame.
   for (unsigned i = CSI.size(); i != 0; --i) {
     unsigned Reg = CSI[i-1].getReg();
     if (X86::GR64RegClass.contains(Reg) || X86::GR32RegClass.contains(Reg))

test/CodeGen/X86/swiftself.ll

-; RUN: llc -verify-machineinstrs < %s -mtriple=x86_64-unknown-unknown | FileCheck %s
-; RUN: llc -O0 -verify-machineinstrs < %s -mtriple=x86_64-unknown-unknown | FileCheck --check-prefix=CHECK-O0 %s
-; RUN: llc -verify-machineinstrs < %s -march=x86 -mcpu=yonah -mtriple=i386-apple-darwin | FileCheck --check-prefix=CHECK-i386 %s
-; RUN: llc -O0 -verify-machineinstrs < %s -march=x86 -mcpu=yonah -mtriple=i386-apple-darwin | FileCheck --check-prefix=CHECK-i386-O0 %s
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown-unknown -o - %s | FileCheck --check-prefix=CHECK --check-prefix=OPT %s
+; RUN: llc -O0 -verify-machineinstrs -mtriple=x86_64-unknown-unknown -o - %s | FileCheck --check-prefix=CHECK %s
 
-; Parameter with swiftself should be allocated to r10.
-define void @check_swiftself(i32* swiftself %addr0) {
-; CHECK-LABEL: check_swiftself:
-; CHECK-O0-LABEL: check_swiftself:
-; CHECK-i386-LABEL: check_swiftself:
-; CHECK-i386-O0-LABEL: check_swiftself:
-
-  %val0 = load volatile i32, i32* %addr0
-; CHECK: movl (%r10),
-; CHECK-O0: movl (%r10),
-; CHECK-i386: movl {{[0-9a-f]+}}(%esp)
-; CHECK-i386-O0: movl {{[0-9a-f]+}}(%esp)
-  ret void
+; Parameter with swiftself should be allocated to r12.
+; CHECK-LABEL: swiftself_param:
+; CHECK: movq %r12, %rax
+define i8 *@swiftself_param(i8* swiftself %addr0) {
+    ret i8 *%addr0
 }
 
-@var8_3 = global i8 0
-declare void @take_swiftself(i8* swiftself %addr0)
-
-define void @simple_args() {
-; CHECK-LABEL: simple_args:
-; CHECK-O0-LABEL: simple_args:
-; CHECK-i386-LABEL: simple_args:
-; CHECK-i386-O0-LABEL: simple_args:
+; Check that r12 is used to pass a swiftself argument.
+; CHECK-LABEL: call_swiftself:
+; CHECK: movq %rdi, %r12
+; CHECK: callq {{_?}}swiftself_param
+define i8 *@call_swiftself(i8* %arg) {
+  %res = call i8 *@swiftself_param(i8* swiftself %arg)
+  ret i8 *%res
+}
 
-  call void @take_swiftself(i8* @var8_3)
-; CHECK: movl {{.*}}, %r10d
-; CHECK: callq {{_?}}take_swiftself
-; CHECK-O0: movabsq {{.*}}, %r10
-; CHECK-O0: callq {{_?}}take_swiftself
-; CHECK-i386: movl {{.*}}, (%esp)
-; CHECK-i386: calll {{.*}}take_swiftself
-; CHECK-i386-O0: movl {{.*}}, (%esp)
-; CHECK-i386-O0: calll {{.*}}take_swiftself
+; r12 should be saved by the callee even if used for swiftself
+; CHECK-LABEL: swiftself_clobber:
+; CHECK: pushq %r12
+; ...
+; CHECK: popq %r12
+define i8 *@swiftself_clobber(i8* swiftself %addr0) {
+  call void asm sideeffect "nop", "~{r12}"()
+  ret i8 *%addr0
+}
 
+; Demonstrate that we do not need any movs when calling multiple functions
+; with swiftself argument.
+; CHECK-LABEL: swiftself_passthrough:
+; CHECK-OPT: callq {{_?}}swiftself_param
+; CHECK-OPT-NEXT: callq {{_?}}swiftself_param
+define void @swiftself_passthrough(i8* swiftself %addr0) {
+  call i8 *@swiftself_param(i8* swiftself %addr0)
+  call i8 *@swiftself_param(i8* swiftself %addr0)
   ret void
 }