AArch64: Use a callee save registers for swiftself parameters

[MatzeB]

Summary

It is very likely that the swiftself parameter is alive throughout most
functions. Putting it into a callee save register should avoid spills
for callers with only a few extra spills in 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.

[manmanren]

Thanks for cleaning up the testing case!

Manman

lib/Target/AArch64/AArch64FrameLowering.cpp

704

So if isReturnAddressTaken is true, LRLiveIn will be true as well?
The original check of !(LRLiveIn && MF.getFrameInfo()->isReturnAddressTaken()) is equivalent to !LRLiveIn?

test/CodeGen/AArch64/swiftself.ll

59

Are these the spills and reloads you mentioned in the summary?

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

[MatzeB]

lib/Target/AArch64/AArch64FrameLowering.cpp

704

If LR is used to access the return address, then isLiveIn(LR) will be true, which is fine. The new code is slightly more allowing since it does not explicitly check for isReturnAddressTaken() anymore. However I don't see how this could ever be marked in otherwise. (Also missing kill flags are not a correctness problem but can just result in worse register scavenging results, though with the reserved LR register this is not an issue anyway).

test/CodeGen/AArch64/swiftself.ll

59

Yep that is what I mentioned, I should probably remove them from the test, as that is not what we want to test here.
And indeed we should find a way to avoid them longer term (I've seen that you worked on SplitCSR before which can be adapted to this case I assume...)

[MatzeB]

I just realized that there are radars that propose x19 to be used for swifterror. We can of course change the register. The important parts are the bits that allow to pass values in callee saved registers.

[MatzeB]

Update testcase to be in line with the other swiftself patches.

[MatzeB]

Add comment that omitting the kill flag is conservatively correct.

[MatzeB]

Update patch: We have to tail calls when the argument passed in the swiftself of the callee is not the same as the swift self of the caller.

[MatzeB]

Move from X19 to X20 to avoid clash with swifterror parameter.

[qcolombet]

Hi Matthias,

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

Cheers,
-Quentin

test/CodeGen/AArch64/swiftself.ll

40

Is it intended that we just check the label for O0?

49

Ditto.

59

Check that addr1 is properly copied into x20 from x1?

[t.p.northover]

The AArch64 changes look fine to me too. I'll leave it to you and Quentin to sort out the best tests, I don't feel too strongly either way.

Tim.

[MatzeB]

Thanks for the review.

test/CodeGen/AArch64/swiftself.ll

40

The label is always checked.
The rest of the check makes sure that we can indeed create code with no additional mov instructions for the paramaters, -O0 codegen just doesn't optimize enough for it and is left out.

49

Ditto (only the optimized codegen performs tail call opts which we want to test for).

[MatzeB]

This landed as r266251, accept & close.

[MatzeB]

This landed as r266251, accept & close.

lib/Target/AArch64/AArch64CallingConvention.td

def CC_AArch64_AAPCS : CallingConv<[
   // slot is 64-bit.
   CCIfByVal<CCPassByVal<8, 8>>,
 
   // The 'nest' parameter, if any, is passed in X18.
   // Darwin uses X18 as the platform register and hence 'nest' isn't currently
   // supported there.
   CCIfNest<CCAssignToReg<[X18]>>,
 
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i64], CCAssignToRegWithShadow<[X20], [W20]>>>,
+
   CCIfConsecutiveRegs<CCCustom<"CC_AArch64_Custom_Block">>,
 
   // Handle i1, i8, i16, i32, i64, f32, f64 and v2f64 by passing in registers,
   // up to eight each of GPR and FPR.
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i32], CCAssignToRegWithShadow<[W0, W1, W2, W3, W4, W5, W6, W7],
                                           [X0, X1, X2, X3, X4, X5, X6, X7]>>,
   // i128 is split to two i64s, we can't fit half to register X7.
def CC_AArch64_DarwinPCS : CallingConv<[
 
   // An SRet is passed in X8, not X0 like a normal pointer parameter.
   CCIfSRet<CCIfType<[i64], CCAssignToRegWithShadow<[X8], [W8]>>>,
 
   // Put ByVal arguments directly on the stack. Minimum size and alignment of a
   // slot is 64-bit.
   CCIfByVal<CCPassByVal<8, 8>>,
 
-  // A SwiftSelf is passed in X9.
-  CCIfSwiftSelf<CCIfType<[i64], CCAssignToRegWithShadow<[X9], [W9]>>>,
+  // Pass SwiftSelf in a callee saved register.
+  CCIfSwiftSelf<CCIfType<[i64], CCAssignToRegWithShadow<[X20], [W20]>>>,
 
   // A SwiftError is passed in X19.
   CCIfSwiftError<CCIfType<[i64], CCAssignToRegWithShadow<[X19], [W19]>>>,
 
   CCIfConsecutiveRegs<CCCustom<"CC_AArch64_Custom_Block">>,
 
   // Handle i1, i8, i16, i32, i64, f32, f64 and v2f64 by passing in registers,
   // up to eight each of GPR and FPR.

lib/Target/AArch64/AArch64FrameLowering.cpp

else {
     if (canUseRedZone(MF))
       Offset -= AFI->getLocalStackSize();
   }
 
   return Offset;
 }
 
 static unsigned getPrologueDeath(MachineFunction &MF, unsigned Reg) {
-  if (Reg != AArch64::LR)
-    return getKillRegState(true);
-
-  // LR maybe referred to later by an @llvm.returnaddress intrinsic.
-  bool LRLiveIn = MF.getRegInfo().isLiveIn(AArch64::LR);
-  bool LRKill = !(LRLiveIn && MF.getFrameInfo()->isReturnAddressTaken());
-  return getKillRegState(LRKill);
+  // 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 IsLiveIn = MF.getRegInfo().isLiveIn(Reg);
+  return getKillRegState(!IsLiveIn);
 }
 
 static bool produceCompactUnwindFrame(MachineFunction &MF) {
   const AArch64Subtarget &Subtarget = MF.getSubtarget<AArch64Subtarget>();
   AttributeSet Attrs = MF.getFunction()->getAttributes();
   return Subtarget.isTargetMachO() &&
          !(Subtarget.getTargetLowering()->supportSwiftError() &&
            Attrs.hasAttrSomewhere(Attribute::SwiftError));

lib/Target/AArch64/AArch64ISelLowering.cpp

bool AArch64TargetLowering::isEligibleForTailCallOptimization(
   }
 
   // Check that the call results are passed in the same way.
   if (!CCState::resultsCompatible(CalleeCC, CallerCC, MF, C, Ins,
                                   CCAssignFnForCall(CalleeCC, isVarArg),
                                   CCAssignFnForCall(CallerCC, isVarArg)))
     return false;
   // The callee has to preserve all registers the caller needs to preserve.
-  if (!CCMatch) {
-    const AArch64RegisterInfo *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;
   }
 
   // Nothing more to check if the callee is taking no arguments
   if (Outs.empty())
     return true;
 
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
 
   CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CalleeCC, isVarArg));
 
   const AArch64FunctionInfo *FuncInfo = MF.getInfo<AArch64FunctionInfo>();
 
-  // If the stack arguments for this call would fit into our own save area then
-  // the call can be made tail.
-  return CCInfo.getNextStackOffset() <= FuncInfo->getBytesInStackArgArea();
+  // If the stack arguments for this call do not fit into our own save area then
+  // the call cannot be made tail.
+  if (CCInfo.getNextStackOffset() > FuncInfo->getBytesInStackArgArea())
+    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();
+    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;
+    }
+  }
+
+  return true;
 }
 
 SDValue AArch64TargetLowering::addTokenForArgument(SDValue Chain,
                                                    SelectionDAG &DAG,
                                                    MachineFrameInfo *MFI,
                                                    int ClobberedFI) const {
   SmallVector<SDValue, 8> ArgChains;
   int64_t FirstByte = MFI->getObjectOffset(ClobberedFI);

test/CodeGen/AArch64/swiftself.ll

-; RUN: llc -verify-machineinstrs < %s -mtriple=aarch64-apple-ios | FileCheck --check-prefix=CHECK-APPLE %s
-; RUN: llc -O0 -verify-machineinstrs < %s -mtriple=aarch64-apple-ios | FileCheck --check-prefix=CHECK-O0 %s
-
-; Parameter with swiftself should be allocated to x9.
-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 w{{.*}}, [x9]
-; CHECK-O0: ldr w{{.*}}, [x9]
-    ret void
+; RUN: llc -verify-machineinstrs -mtriple=aarch64-apple-ios -o - %s | FileCheck --check-prefix=CHECK --check-prefix=OPT %s
+; RUN: llc -O0 -verify-machineinstrs -mtriple=aarch64-apple-ios -o - %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=aarch64-unknown-linux-gnu -o - %s | FileCheck --check-prefix=CHECK --check-prefix=OPT %s
+
+; Parameter with swiftself should be allocated to x20.
+; CHECK-LABEL: swiftself_param:
+; CHECK: mov x0, x20
+; CHECK-NEXT: ret
+define i8* @swiftself_param(i8* swiftself %addr0) {
+  ret i8 *%addr0
 }
 
-@var8_3 = global i8 0
-declare void @take_swiftself(i8* swiftself %addr0)
+; Check that x20 is used to pass a swiftself argument.
+; CHECK-LABEL: call_swiftself:
+; CHECK: mov x20, x0
+; CHECK: bl {{_?}}swiftself_param
+; CHECK: ret
+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 x9,
-; CHECK-APPLE: bl {{_?}}take_swiftself
-; CHECK-O0: add x9,
-; CHECK-O0: bl {{_?}}take_swiftself
+; x20 should be saved by the callee even if used for swiftself
+; CHECK-LABEL: swiftself_clobber:
+; CHECK: {{stp|str}} {{.*}}x20{{.*}}sp
+; ...
+; CHECK: {{ldp|ldr}} {{.*}}x20{{.*}}sp
+; CHECK: ret
+define i8 *@swiftself_clobber(i8* swiftself %addr0) {
+  call void asm sideeffect "", "~{x20}"()
+  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
+; OPT: ret
+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: b {{_?}}swiftself_param
+; OPT-NOT: ret
+define i8* @swiftself_tail(i8* swiftself %addr0) {
+  call void asm sideeffect "", "~{x20}"()
+  %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: bl {{_?}}swiftself_param
+; CHECK: ret
+define i8* @swiftself_notail(i8* swiftself %addr0, i8* %addr1) nounwind {
+  %res = tail call i8* @swiftself_param(i8* swiftself %addr1)
+  ret i8* %res
+}