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

300–301

Type: Should be SwiftSelf instead of SwiftSwlf.

300–301

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

Invert the condition and continue (LLVM coding style).

3861

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

tabs :)

3869

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

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 SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i64], CCAssignToReg<[R13]>>>,
 
   // A SwiftError is passed in R12.
   CCIfSwiftError<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 ]>>,
 

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.
+    // Omitting the kill flags is conservatively correct even if the live-in
+    // is not used after all.
+    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))

lib/Target/X86/X86ISelLowering.cpp

if (Unused) {
     }
   }
 
   // Check that the call results are passed in the same way.
   if (!CCState::resultsCompatible(CalleeCC, CallerCC, MF, C, Ins,
                                   RetCC_X86, RetCC_X86))
     return false;
   // The callee has to preserve all registers the caller needs to preserve.
-  if (!CCMatch) {
-    const X86RegisterInfo *TRI = Subtarget.getRegisterInfo();
-    if (!TRI->regmaskSubsetEqual(TRI->getCallPreservedMask(MF, CallerCC),
-                                 TRI->getCallPreservedMask(MF, CalleeCC)))
+  const uint32_t *CallerPreserved = TRI->getCallPreservedMask(MF, CallerCC);
+  if (!CCMatch) {
+    const uint32_t *CalleePreserved = TRI->getCallPreservedMask(MF, CalleeCC);
+    if (!TRI->regmaskSubsetEqual(CallerPreserved, CalleePreserved))
       return false;
   }
 
   unsigned StackArgsSize = 0;
 
   // If the callee takes no arguments then go on to check the results of the
   // call.
   if (!Outs.empty()) {
if (!Subtarget.is64Bit() &&
         default: break;
         case X86::EAX: case X86::EDX: case X86::ECX:
           if (++NumInRegs == MaxInRegs)
             return false;
           break;
         }
       }
     }
+
+    // Parameters passed in callee saved registers must have the same value in
+    // caller and callee.
+    for (unsigned I = 0, E = ArgLocs.size(); I != E; ++I) {
+      const CCValAssign &ArgLoc = ArgLocs[I];
+      if (!ArgLoc.isRegLoc())
+  continue;
+      unsigned Reg = ArgLoc.getLocReg();
+      if (!MachineOperand::clobbersPhysReg(CallerPreserved, Reg)) {
+  // Check that we pass the value used for the caller.
+  SDValue Value = OutVals[I];
+  if (Value->getOpcode() != ISD::CopyFromReg)
+    return false;
+  unsigned ArgReg = cast<RegisterSDNode>(Value->getOperand(1))->getReg();
+  const MachineRegisterInfo &MRI = MF.getRegInfo();
+  if (MRI.getLiveInPhysReg(ArgReg) != Reg)
+    return false;
+      }
+    }
   }
 
   bool CalleeWillPop =
       X86::isCalleePop(CalleeCC, Subtarget.is64Bit(), isVarArg,
                        MF.getTarget().Options.GuaranteedTailCallOpt);
 
   if (unsigned BytesToPop =
           MF.getInfo<X86MachineFunctionInfo>()->getBytesToPopOnReturn()) {

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
-
-; 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
+; 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 %s
+
+; Parameter with swiftself should be allocated to r13.
+; CHECK-LABEL: swiftself_param:
+; CHECK: movq %r13, %rax
+define i8 *@swiftself_param(i8* swiftself %addr0) {
+    ret i8 *%addr0
 }
 
-@var8_3 = global i8 0
-declare void @take_swiftself(i8* swiftself %addr0)
+; Check that r13 is used to pass a swiftself argument.
+; CHECK-LABEL: call_swiftself:
+; CHECK: movq %rdi, %r13
+; CHECK: callq {{_?}}swiftself_param
+define i8 *@call_swiftself(i8* %arg) {
+  %res = call i8 *@swiftself_param(i8* swiftself %arg)
+  ret i8 *%res
+}
 
-define void @simple_args() {
-; CHECK-LABEL: simple_args:
-; CHECK-O0-LABEL: simple_args:
-; CHECK-i386-LABEL: simple_args:
-; CHECK-i386-O0-LABEL: simple_args:
-
-  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
+; r13 should be saved by the callee even if used for swiftself
+; CHECK-LABEL: swiftself_clobber:
+; CHECK: pushq %r13
+; ...
+; CHECK: popq %r13
+define i8 *@swiftself_clobber(i8* swiftself %addr0) {
+  call void asm sideeffect "nop", "~{r13}"()
+  ret i8 *%addr0
+}
 
+; Demonstrate that we do not need any movs when calling multiple functions
+; with swiftself argument.
+; CHECK-LABEL: swiftself_passthrough:
+; OPT: callq {{_?}}swiftself_param
+; 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
 }
+
+; We can use a tail call if the callee swiftself is the same as the caller one.
+; CHECK-LABEL: swiftself_tail:
+; OPT: jmp {{_?}}swiftself_param
+; OPT-NOT: ret
+define i8* @swiftself_tail(i8* swiftself %addr0) {
+  call void asm sideeffect "", "~{r13}"()
+  %res = tail call i8* @swiftself_param(i8* swiftself %addr0)
+  ret i8* %res
+}
+
+; We can not use a tail call if the callee swiftself is not the same as the
+; caller one.
+; CHECK-LABEL: swiftself_notail:
+; CHECK: callq {{_?}}swiftself_param
+; CHECK: retq
+define i8* @swiftself_notail(i8* swiftself %addr0, i8* %addr1) nounwind {
+  %res = tail call i8* @swiftself_param(i8* swiftself %addr1)
+  ret i8* %res
+}