ARM: 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]

test/CodeGen/ARM/swiftself.ll

2 ↗

(On Diff #53039)

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

[MatzeB]

Remove unnecessary --check-prefix.

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

914 ↗

(On Diff #53039)

Why not just make:

bool isKill = !MF.getRegInfo().isLiveIn(Reg);

[t.p.northover]

test/CodeGen/ARM/swiftself.ll

2 ↗

(On Diff #53044)

Why? All labels are under CHECK; it's a fairly common FileCheck idiom to have a shared check-prefix and a run-specific one to save duplication.

But I think there actually is an issue here: OPT != CHECK-OPT.

[t.p.northover]

test/CodeGen/ARM/swiftself.ll

2 ↗

(On Diff #53044)

Sorry, I was looking at a newer diff. You were right about the original. I think OPT is still problematic though.

[MatzeB]

Avoid extra isKill variable

[MatzeB]

Fix wrong "OPT:" check lines in test.

[qcolombet]

lib/Target/ARM/ARMFrameLowering.cpp

914 ↗

(On Diff #53049)

Because if the register is live we *must not* add it into the live-in.

925 ↗

(On Diff #53049)

Same comment as x86: conservatively correct.

[MatzeB]

Add note that omitting a kill flag is correct.

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

914 ↗

(On Diff #53068)

yeah, I meant it as a duplication. :)

test/CodeGen/ARM/swiftself.ll

2 ↗

(On Diff #53068)

The CHECK prefix is still there... already assumed by default.

[MatzeB]

lib/Target/ARM/ARMFrameLowering.cpp

914 ↗

(On Diff #53046)

I found the codes intention is clearer if the if() condition looks at the live-in not at something called isKill. What about this new version that drops the isKill variable instead?

test/CodeGen/ARM/swiftself.ll

2 ↗

(On Diff #53068)

static void AddCheckPrefixIfNeeded() {
  if (CheckPrefixes.empty())
    CheckPrefixes.push_back("CHECK");
}

It is only default if you do not add any other check-prefixes.

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

914 ↗

(On Diff #53068)

looks good

test/CodeGen/ARM/swiftself.ll

2 ↗

(On Diff #53068)

Hum, you're right... I assumed it always did... :(

[MatzeB]

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

[MatzeB]

Consistently add SwiftSelf support to all calling conventions that support SwiftError.

[rengolin]

lib/Target/ARM/ARMISelLowering.cpp

2225 ↗

(On Diff #53359)

This part seems oddly formatted...

[MatzeB]

Replace sneaky tabs with spaces.

[rengolin]

Right, this looks good to me, but I think it should go together with the AArch64 changes.

[qcolombet]

Hi Matthias,

Same comment as http://reviews.llvm.org/D18902 + some questions on the test cases.

Cheers,
-Quentin

test/CodeGen/ARM/swiftself.ll

38 ↗

(On Diff #53434)

No checks for O0?

48 ↗

(On Diff #53434)

No checks for O0 and OPT alone?

[MatzeB]

Thanks for the reviews.

test/CodeGen/ARM/swiftself.ll

38 ↗

(On Diff #53434)

This checks whether we can indeed generate code without any additional movs between the calls. The O0 codegen does not optimize enough for that so there is nothing to test.

48 ↗

(On Diff #53434)

This checks that tail call optimization does indeed happen when the params are the same, O0 codegen does not perform tailcall opts, "armv7-apple-ios" also has tailcalls disabled so I introduced a new TAILCALL prefix for all target where it does happen.

llvm/trunk/lib/Target/ARM/ARMCallingConv.td

 //===----------------------------------------------------------------------===//
 def CC_ARM_APCS : CallingConv<[
 
   // Handles byval parameters.
   CCIfByVal<CCPassByVal<4, 4>>,
     
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
 
-  // A SwiftSelf is passed in R9.
-  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R9]>>>,
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R10]>>>,
 
   // A SwiftError is passed in R6.
   CCIfSwiftError<CCIfType<[i32], CCAssignToReg<[R6]>>>,
 
   // Handle all vector types as either f64 or v2f64.
   CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
   CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,
 
   // f64 and v2f64 are passed in adjacent GPRs, possibly split onto the stack
   CCIfType<[f64, v2f64], CCCustom<"CC_ARM_APCS_Custom_f64">>,
 
   CCIfType<[f32], CCBitConvertToType<i32>>,
   CCIfType<[i32], CCAssignToReg<[R0, R1, R2, R3]>>,
 
   CCIfType<[i32], CCAssignToStack<4, 4>>,
   CCIfType<[f64], CCAssignToStack<8, 4>>,
   CCIfType<[v2f64], CCAssignToStack<16, 4>>
 ]>;
 
 def RetCC_ARM_APCS : CallingConv<[
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[f32], CCBitConvertToType<i32>>,
 
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R10]>>>,
+
   // A SwiftError is returned in R6.
   CCIfSwiftError<CCIfType<[i32], CCAssignToReg<[R6]>>>,
 
   // Handle all vector types as either f64 or v2f64.
   CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
   CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,
 
   CCIfType<[f64, v2f64], CCCustom<"RetCC_ARM_APCS_Custom_f64">>,
def CC_ARM_AAPCS : CallingConv<[
 
   // The 'nest' parameter, if any, is passed in R12.
   CCIfNest<CCAssignToReg<[R12]>>,
 
   // Handle all vector types as either f64 or v2f64.
   CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
   CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,
 
-  // A SwiftSelf is passed in R9.
-  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R9]>>>,
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R10]>>>,
 
   // A SwiftError is passed in R6.
   CCIfSwiftError<CCIfType<[i32], CCAssignToReg<[R6]>>>,
 
   CCIfType<[f64, v2f64], CCCustom<"CC_ARM_AAPCS_Custom_f64">>,
   CCIfType<[f32], CCBitConvertToType<i32>>,
   CCDelegateTo<CC_ARM_AAPCS_Common>
 ]>;
 
 def RetCC_ARM_AAPCS : CallingConv<[
   // Handle all vector types as either f64 or v2f64.
   CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
   CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,
 
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R10]>>>,
+
   // A SwiftError is returned in R6.
   CCIfSwiftError<CCIfType<[i32], CCAssignToReg<[R6]>>>,
 
   CCIfType<[f64, v2f64], CCCustom<"RetCC_ARM_AAPCS_Custom_f64">>,
   CCIfType<[f32], CCBitConvertToType<i32>>,
   CCDelegateTo<RetCC_ARM_AAPCS_Common>
 ]>;
 
 //===----------------------------------------------------------------------===//
 // ARM AAPCS-VFP (EABI) Calling Convention
 // Also used for FastCC (when VFP2 or later is available)
 //===----------------------------------------------------------------------===//
 
 def CC_ARM_AAPCS_VFP : CallingConv<[
   // Handles byval parameters.
   CCIfByVal<CCPassByVal<4, 4>>,
 
   // Handle all vector types as either f64 or v2f64.
   CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
   CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,
 
-  // A SwiftSelf is passed in R9.
-  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R9]>>>,
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R10]>>>,
 
   // A SwiftError is passed in R6.
   CCIfSwiftError<CCIfType<[i32], CCAssignToReg<[R6]>>>,
 
   // HFAs are passed in a contiguous block of registers, or on the stack
   CCIfConsecutiveRegs<CCCustom<"CC_ARM_AAPCS_Custom_Aggregate">>,
 
   CCIfType<[v2f64], CCAssignToReg<[Q0, Q1, Q2, Q3]>>,
   CCIfType<[f64], CCAssignToReg<[D0, D1, D2, D3, D4, D5, D6, D7]>>,
   CCIfType<[f32], CCAssignToReg<[S0, S1, S2, S3, S4, S5, S6, S7, S8,
                                  S9, S10, S11, S12, S13, S14, S15]>>,
   CCDelegateTo<CC_ARM_AAPCS_Common>
 ]>;
 
 def RetCC_ARM_AAPCS_VFP : CallingConv<[
   // Handle all vector types as either f64 or v2f64.
   CCIfType<[v1i64, v2i32, v4i16, v8i8, v2f32], CCBitConvertToType<f64>>,
   CCIfType<[v2i64, v4i32, v8i16, v16i8, v4f32], CCBitConvertToType<v2f64>>,
 
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i32], CCAssignToReg<[R10]>>>,
+
   // A SwiftError is returned in R6.
   CCIfSwiftError<CCIfType<[i32], CCAssignToReg<[R6]>>>,
 
   CCIfType<[v2f64], CCAssignToReg<[Q0, Q1, Q2, Q3]>>,
   CCIfType<[f64], CCAssignToReg<[D0, D1, D2, D3, D4, D5, D6, D7]>>,
   CCIfType<[f32], CCAssignToReg<[S0, S1, S2, S3, S4, S5, S6, S7, S8,
                                  S9, S10, S11, S12, S13, S14, S15]>>,
   CCDelegateTo<RetCC_ARM_AAPCS_Common>

llvm/trunk/lib/Target/ARM/ARMFrameLowering.cpp

while (i != 0) {
     for (; i != 0; --i) {
       unsigned Reg = CSI[i-1].getReg();
       if (!(Func)(Reg, STI.isTargetMachO())) continue;
 
       // D-registers in the aligned area DPRCS2 are NOT spilled here.
       if (Reg >= ARM::D8 && Reg < ARM::D8 + NumAlignedDPRCS2Regs)
         continue;
 
-      // Add the callee-saved register as live-in unless it's LR and
-      // @llvm.returnaddress is called. If LR is returned for
-      // @llvm.returnaddress then it's already added to the function and
-      // entry block live-in sets.
-      bool isKill = true;
-      if (Reg == ARM::LR) {
-        if (MF.getFrameInfo()->isReturnAddressTaken() &&
-            MF.getRegInfo().isLiveIn(Reg))
-          isKill = false;
-      }
-
-      if (isKill)
+      bool isLiveIn = MF.getRegInfo().isLiveIn(Reg);
+      if (!isLiveIn)
         MBB.addLiveIn(Reg);
-
       // If NoGap is true, push consecutive registers and then leave the rest
       // for other instructions. e.g.
       // vpush {d8, d10, d11} -> vpush {d8}, vpush {d10, d11}
       if (NoGap && LastReg && LastReg != Reg-1)
         break;
       LastReg = Reg;
-      Regs.push_back(std::make_pair(Reg, isKill));
+      // 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.
+      Regs.push_back(std::make_pair(Reg, /*isKill=*/!isLiveIn));
     }
 
     if (Regs.empty())
       continue;
     if (Regs.size() > 1 || StrOpc== 0) {
       MachineInstrBuilder MIB =
         AddDefaultPred(BuildMI(MBB, MI, DL, TII.get(StmOpc), ARM::SP)
                        .addReg(ARM::SP).setMIFlags(MIFlags));

llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::IsEligibleForTailCallOptimization(SDValue Callee,
 
   // Check that the call results are passed in the same way.
   LLVMContext &C = *DAG.getContext();
   if (!CCState::resultsCompatible(CalleeCC, CallerCC, MF, C, Ins,
                                   CCAssignFnForNode(CalleeCC, true, isVarArg),
                                   CCAssignFnForNode(CallerCC, true, isVarArg)))
     return false;
   // The callee has to preserve all registers the caller needs to preserve.
-  if (CalleeCC != CallerCC) {
-    const ARMBaseRegisterInfo *TRI = Subtarget->getRegisterInfo();
-    if (!TRI->regmaskSubsetEqual(TRI->getCallPreservedMask(MF, CallerCC),
-                                 TRI->getCallPreservedMask(MF, CalleeCC)))
+  const uint32_t *CallerPreserved = TRI->getCallPreservedMask(MF, CallerCC);
+  if (CalleeCC != CallerCC) {
+    const uint32_t *CalleePreserved = TRI->getCallPreservedMask(MF, CalleeCC);
+    if (!TRI->regmaskSubsetEqual(CallerPreserved, CalleePreserved))
       return false;
   }
 
   // If Caller's vararg or byval argument has been split between registers and
   // stack, do not perform tail call, since part of the argument is in caller's
   // local frame.
   const ARMFunctionInfo *AFI_Caller = MF.getInfo<ARMFunctionInfo>();
   if (AFI_Caller->getArgRegsSaveSize())
if (CCInfo.getNextStackOffset()) {
           }
         } else if (!VA.isRegLoc()) {
           if (!MatchingStackOffset(Arg, VA.getLocMemOffset(), Flags,
                                    MFI, MRI, TII))
             return false;
         }
       }
     }
+
+    // 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();
+      // Only look at callee saved registers.
+      if (MachineOperand::clobbersPhysReg(CallerPreserved, Reg))
+        continue;
+      // Check that we pass the value used for the caller.
+      // (We look for a CopyFromReg reading a virtual register that is used
+      //  for the function live-in value of register Reg)
+      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;
+    }
   }
 
   return true;
 }
 
 bool
 ARMTargetLowering::CanLowerReturn(CallingConv::ID CallConv,
                                   MachineFunction &MF, bool isVarArg,

llvm/trunk/test/CodeGen/ARM/swiftself.ll

-; RUN: llc -verify-machineinstrs < %s -mtriple=armv7k-apple-ios8.0 -mcpu=cortex-a7 | FileCheck --check-prefix=CHECK-APPLE %s
-; RUN: llc -O0 -verify-machineinstrs < %s -mtriple=armv7k-apple-ios8.0 -mcpu=cortex-a7 | FileCheck --check-prefix=CHECK-O0 %s
+; RUN: llc -verify-machineinstrs -mtriple=armv7k-apple-ios8.0 -mcpu=cortex-a7 -o - %s | FileCheck --check-prefix=CHECK --check-prefix=OPT --check-prefix=TAILCALL %s
+; RUN: llc -O0 -verify-machineinstrs -mtriple=armv7k-apple-ios8.0 -mcpu=cortex-a7 -o - %s | FileCheck %s
 
-; RUN: llc -verify-machineinstrs < %s -mtriple=armv7-apple-ios | FileCheck --check-prefix=CHECK-APPLE %s
-; RUN: llc -O0 -verify-machineinstrs < %s -mtriple=armv7-apple-ios | FileCheck --check-prefix=CHECK-O0 %s
+; RUN: llc -verify-machineinstrs -mtriple=armv7-apple-ios -o - %s | FileCheck --check-prefix=CHECK --check-prefix=OPT %s
+; RUN: llc -O0 -verify-machineinstrs -mtriple=armv7-apple-ios -o - %s | FileCheck %s
 
-; Parameter with swiftself should be allocated to r9.
-define void @check_swiftself(i32* swiftself %addr0) {
-; CHECK-APPLE-LABEL: check_swiftself:
-; CHECK-O0-LABEL: check_swiftself:
-
-    %val0 = load volatile i32, i32* %addr0
-; CHECK-APPLE: ldr r{{.*}}, [r9]
-; CHECK-O0: ldr r{{.*}}, [r9]
-    ret void
+; Parameter with swiftself should be allocated to r10.
+; CHECK-LABEL: swiftself_param:
+; CHECK: mov r0, r10
+define i8 *@swiftself_param(i8* swiftself %addr0) {
+    ret i8 *%addr0
 }
 
-@var8_3 = global i8 0
-declare void @take_swiftself(i8* swiftself %addr0)
+; Check that r10 is used to pass a swiftself argument.
+; CHECK-LABEL: call_swiftself:
+; CHECK: mov r10, r0
+; CHECK: bl {{_?}}swiftself_param
+define i8 *@call_swiftself(i8* %arg) {
+  %res = call i8 *@swiftself_param(i8* swiftself %arg)
+  ret i8 *%res
+}
 
-define void @simple_args() {
-; CHECK-APPLE-LABEL: simple_args:
-; CHECK-O0-LABEL: simple_args:
-
-  call void @take_swiftself(i8* @var8_3)
-; CHECK-APPLE: add r9, pc
-; CHECK-APPLE: bl {{_?}}take_swiftself
-; CHECK-O0: add r9, pc
-; CHECK-O0: bl {{_?}}take_swiftself
+; r10 should be saved by the callee even if used for swiftself
+; CHECK-LABEL: swiftself_clobber:
+; CHECK: push {r10}
+; ...
+; CHECK: pop {r10}
+define i8 *@swiftself_clobber(i8* swiftself %addr0) {
+  call void asm sideeffect "", "~{r10}"()
+  ret i8 *%addr0
+}
 
+; Demonstrate that we do not need any movs when calling multiple functions
+; with swiftself argument.
+; CHECK-LABEL: swiftself_passthrough:
+; OPT-NOT: mov{{.*}}r10
+; OPT: bl {{_?}}swiftself_param
+; OPT-NOT: mov{{.*}}r10
+; OPT-NEXT: bl {{_?}}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:
+; TAILCALL: b {{_?}}swiftself_param
+; TAILCALL-NOT: pop
+define i8* @swiftself_tail(i8* swiftself %addr0) {
+  call void asm sideeffect "", "~{r10}"()
+  %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: mov r10, r0
+; CHECK: bl {{_?}}swiftself_param
+; CHECK: pop
+define i8* @swiftself_notail(i8* swiftself %addr0, i8* %addr1) nounwind {
+  %res = tail call i8* @swiftself_param(i8* swiftself %addr1)
+  ret i8* %res
+}