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

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

[MatzeB]

Remove unnecessary --check-prefix.

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

914

Why not just make:

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

[t.p.northover]

test/CodeGen/ARM/swiftself.ll

2

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

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

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

925

Same comment as x86: conservatively correct.

[MatzeB]

Add note that omitting a kill flag is correct.

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

914

yeah, I meant it as a duplication. :)

test/CodeGen/ARM/swiftself.ll

2

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

[MatzeB]

lib/Target/ARM/ARMFrameLowering.cpp

914

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

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

looks good

test/CodeGen/ARM/swiftself.ll

2

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

19

No checks for O0?

27

No checks for O0 and OPT alone?

[MatzeB]

Thanks for the reviews.

test/CodeGen/ARM/swiftself.ll

19

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.

27

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.

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]>>>,
 
   // 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">>,
 
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]>>>,
 
   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.

 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]>>>,
 
   // 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]>>,

lib/Target/ARM/ARMFrameLowering.cpp

while (i != 0) {
     for (; i != 0; --i) {
       unsigned Reg = CSI[i-1].getReg();
       if (!(Func)(Reg, STI.isTargetDarwin())) 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));

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 %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)
-
-define void @simple_args() {
-; CHECK-APPLE-LABEL: simple_args:
-; CHECK-O0-LABEL: simple_args:
+; 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
+}
 
-  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: bl {{_?}}swiftself_param
+; 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
 }