[AArch64] Break the dependency between FP and SP when possible.

[hiraditya]

Summary

In collaboration with Evandro Menezes.

When the SP in not changed because of realignment/VLAs etc., we restore the SP by using the previous value of SP and not the FP. Breaking the dependency will help in cases when
the epilog of a callee is close to the epilog of the caller; for then "sub sp, fp, #" depends on the load restoring the FP in the epilog of the callee.

[mcrosier]

lib/Target/AArch64/AArch64FrameLowering.h

22 ↗

(On Diff #50328)

stackRealigned should be capitalized.

[hiraditya]

Capitalized the member variable.

[t.p.northover]

Hi,

This looks like a big hammer for a really weird corner case. How often is a write to fp close enough to the epilogue for this to matter? If it really is a frame pointer in that function, it'll be last written in the prologue; and if it's not the frame pointer it can't be liveout so aren't you likely to have some credible use of fp between the def and the epilogue anyway?

Cheers.

Tim.

lib/Target/AArch64/AArch64FrameLowering.h

22 ↗

(On Diff #50334)

This should be in AArch64MachineFunctionInfo.h too. Adding the very first mutable variable to an otherwise completely empty class should really have been a warning flag.

[evandro]

In D18060#372344, @t.p.northover wrote:,

This looks like a big hammer for a really weird corner case. How often is a write to fp close enough to the epilogue for this to matter?

It's not when the prolog is close to the epilog, but when the epilog of a callee is close to the epilog of the caller, for then "sub sp, fp, #" depends on the load restoring the FP in the epilog of the callee, which is typically on the long side, even if it does not miss.

[t.p.northover]

Ah, that makes more sense. Thanks.

[hiraditya]

lib/Target/AArch64/AArch64FrameLowering.h

22 ↗

(On Diff #50334)

Thanks for the feedback. I'll try to find more appropriate place to put this variable.

[gberry]

Could you add a bit more description to the commit message along the lines of your response to Tim's comment about the motivation for this change?

lib/Target/AArch64/AArch64FrameLowering.cpp

612

Minor nit: you might want to swap the order of these so you do the MFI load conditionally.

[hiraditya]

Moved the variable StackRealigned to AArch64MachineFunctionInfo.h

[t.p.northover]

lib/Target/AArch64/AArch64FrameLowering.cpp

298

Isn't this redundant now that it's in per-function storage?

[hiraditya]

Removed redundant assignment.

[t.p.northover]

I think this is good now. Thanks for the updates.

Tim.

[hiraditya]

Hi Tim,
Thanks for accepting the patch. Could you please commit this patch as I do not have commit approval.

Thanks,
-Aditya

[mcrosier]

Committed r263458.

lib/Target/AArch64/AArch64FrameLowering.cpp

void AArch64FrameLowering::emitPrologue(MachineFunction &MF,
   const Function *Fn = MF.getFunction();
   const AArch64Subtarget &Subtarget = MF.getSubtarget<AArch64Subtarget>();
   const AArch64RegisterInfo *RegInfo = Subtarget.getRegisterInfo();
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
   MachineModuleInfo &MMI = MF.getMMI();
   AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
   bool needsFrameMoves = MMI.hasDebugInfo() || Fn->needsUnwindTableEntry();
   bool HasFP = hasFP(MF);
+  AFI->setStackRealigned(false);
 
   // Debug location must be unknown since the first debug location is used
   // to determine the end of the prologue.
   DebugLoc DL;
 
   // All calls are tail calls in GHC calling conv, and functions have no
   // prologue/epilogue.
   if (MF.getFunction()->getCallingConv() == CallingConv::GHC)
if (NumBytes && NeedsRealignment) {
     uint32_t andMaskEncoded =
         (1                   <<12) // = N
       | ((64-NrBitsToZero)   << 6) // immr
       | ((64-NrBitsToZero-1) << 0) // imms
       ;
     BuildMI(MBB, MBBI, DL, TII->get(AArch64::ANDXri), AArch64::SP)
       .addReg(scratchSPReg, RegState::Kill)
       .addImm(andMaskEncoded);
+    AFI->setStackRealigned(true);
   }
 
   // If we need a base pointer, set it up here. It's whatever the value of the
   // stack pointer is at this point. Any variable size objects will be allocated
   // after this, so we can still use the base pointer to reference locals.
   //
   // FIXME: Clarify FrameSetup flags here.
   // Note: Use emitFrameOffset() like above for FP if the FrameSetup flag is
if (NoCalleeSaveRestore || ArgumentPopSize == 0)
       return;
     NumBytes = 0;
   }
 
   // Restore the original stack pointer.
   // FIXME: Rather than doing the math here, we should instead just use
   // non-post-indexed loads for the restores if we aren't actually going to
   // be able to save any instructions.
-  if (NumBytes || MFI->hasVarSizedObjects())
+  if (MFI->hasVarSizedObjects() || AFI->isStackRealigned())
     emitFrameOffset(MBB, LastPopI, DL, AArch64::SP, AArch64::FP,
                     -AFI->getCalleeSavedStackSize() + 16, TII,
                     MachineInstr::FrameDestroy);
+  else if (NumBytes)
+    emitFrameOffset(MBB, LastPopI, DL, AArch64::SP, AArch64::SP,
+                    NumBytes, TII, MachineInstr::FrameDestroy);
 
   // This must be placed after the callee-save restore code because that code
   // assumes the SP is at the same location as it was after the callee-save save
   // code in the prologue.
   if (ArgumentPopSize)
     emitFrameOffset(MBB, MBB.getFirstTerminator(), DL, AArch64::SP, AArch64::SP,
                     ArgumentPopSize, TII, MachineInstr::FrameDestroy);
 }

lib/Target/AArch64/AArch64MachineFunctionInfo.h

class AArch64FunctionInfo : public MachineFunctionInfo {
   /// \brief Size of the varargs area for arguments passed in floating-point
   /// registers.
   unsigned VarArgsFPRSize;
 
   /// True if this function has a subset of CSRs that is handled explicitly via
   /// copies.
   bool IsSplitCSR;
 
+  /// True when the stack gets realigned dynamically because the size of stack
+  /// frame is unknown at compile time. e.g., in case of VLAs.
+  bool StackRealigned;
 public:
   AArch64FunctionInfo()
       : BytesInStackArgArea(0), ArgumentStackToRestore(0), HasStackFrame(false),
         NumLocalDynamicTLSAccesses(0), VarArgsStackIndex(0), VarArgsGPRIndex(0),
         VarArgsGPRSize(0), VarArgsFPRIndex(0), VarArgsFPRSize(0),
-        IsSplitCSR(false) {}
+        IsSplitCSR(false), StackRealigned(false) {}
 
   explicit AArch64FunctionInfo(MachineFunction &MF)
       : BytesInStackArgArea(0), ArgumentStackToRestore(0), HasStackFrame(false),
         NumLocalDynamicTLSAccesses(0), VarArgsStackIndex(0), VarArgsGPRIndex(0),
         VarArgsGPRSize(0), VarArgsFPRIndex(0), VarArgsFPRSize(0),
-        IsSplitCSR(false) {
+        IsSplitCSR(false), StackRealigned(false) {
     (void)MF;
   }
 
   unsigned getBytesInStackArgArea() const { return BytesInStackArgArea; }
   void setBytesInStackArgArea(unsigned bytes) { BytesInStackArgArea = bytes; }
 
   unsigned getArgumentStackToRestore() const { return ArgumentStackToRestore; }
   void setArgumentStackToRestore(unsigned bytes) {
     ArgumentStackToRestore = bytes;
   }
 
   bool hasStackFrame() const { return HasStackFrame; }
   void setHasStackFrame(bool s) { HasStackFrame = s; }
 
+  bool isStackRealigned() const { return StackRealigned; }
+  void setStackRealigned(bool s) { StackRealigned = s; }
+
   bool isSplitCSR() const { return IsSplitCSR; }
   void setIsSplitCSR(bool s) { IsSplitCSR = s; }
 
   void setLocalStackSize(unsigned Size) { LocalStackSize = Size; }
   unsigned getLocalStackSize() const { return LocalStackSize; }
 
   void setCalleeSavedStackSize(unsigned Size) { CalleeSavedStackSize = Size; }
   unsigned getCalleeSavedStackSize() const { return CalleeSavedStackSize; }

test/CodeGen/AArch64/arm64-hello.ll

 ; RUN: llc < %s -mtriple=arm64-apple-ios7.0 -disable-post-ra -disable-fp-elim | FileCheck %s
 ; RUN: llc < %s -mtriple=arm64-linux-gnu -disable-post-ra | FileCheck %s --check-prefix=CHECK-LINUX
 
 ; CHECK-LABEL: main:
 ; CHECK:  stp x29, x30, [sp, #-16]!
 ; CHECK-NEXT: mov x29, sp
 ; CHECK-NEXT: sub sp, sp, #16
 ; CHECK-NEXT: stur  wzr, [x29, #-4]
 ; CHECK:  adrp  x0, L_.str@PAGE
 ; CHECK:  add x0, x0, L_.str@PAGEOFF
 ; CHECK-NEXT: bl  _puts
-; CHECK-NEXT: mov sp, x29
+; CHECK-NEXT: add sp, sp, #16
 ; CHECK-NEXT: ldp x29, x30, [sp], #16
 ; CHECK-NEXT: ret
 
 ; CHECK-LINUX-LABEL: main:
 ; CHECK-LINUX:  str x30, [sp, #-16]!
 ; CHECK-LINUX-NEXT: sub sp, sp, #16
 ; CHECK-LINUX-NEXT: str wzr, [sp, #12]
 ; CHECK-LINUX:  adrp  x0, .L.str

test/CodeGen/AArch64/arm64-patchpoint.ll

 
 ; Caller frame metadata with stackmaps. This should not be optimized
 ; as a leaf function.
 ;
 ; CHECK-LABEL: caller_meta_leaf
 ; CHECK:       mov x29, sp
 ; CHECK-NEXT:  sub sp, sp, #32
 ; CHECK:       Ltmp
-; CHECK:       mov sp, x29
+; CHECK:       add sp, sp, #32
 ; CHECK:       ret
 
 define void @caller_meta_leaf() {
 entry:
   %metadata = alloca i64, i32 3, align 8
   store i64 11, i64* %metadata
   store i64 12, i64* %metadata
   store i64 13, i64* %metadata

test/CodeGen/AArch64/arm64-shrink-wrapping.ll

 ; CHECK-NEXT: sub x1, [[SAVE_SP]], #4
 ; Set the first argument to zero.
 ; CHECK-NEXT: mov w0, wzr
 ; CHECK-NEXT: bl _doSomething
 ; 
 ; Without shrink-wrapping, epilogue is in the exit block.
 ; DISABLE: [[EXIT_LABEL]]:
 ; Epilogue code.
-; CHECK-NEXT: mov sp, [[SAVE_SP]]
-; CHECK-NEXT: ldp [[SAVE_SP]], [[CSR]], [sp], #16
+; CHECK-NEXT: add sp, sp, #16
+; CHECK-NEXT: ldp x{{[0-9]+}}, [[CSR]], [sp], #16
 ;
 ; With shrink-wrapping, exit block is a simple return.
 ; ENABLE: [[EXIT_LABEL]]:
 ; CHECK-NEXT: ret
 define i32 @foo(i32 %a, i32 %b) {
   %tmp = alloca i32, align 4
   %tmp2 = icmp slt i32 %a, %b
   br i1 %tmp2, label %true, label %false

 ; CHECK-NEXT: lsl w0, w0, #3
 ;
 ; DISABLE: b [[IFEND_LABEL:LBB[0-9_]+]]
 ; DISABLE: [[ELSE_LABEL]]: ; %if.else
 ; DISABLE-NEXT: lsl w0, w1, #1
 ; DISABLE: [[IFEND_LABEL]]: ; %if.end
 ;
 ; Epilogue code.
-; CHECK: mov sp, [[NEW_SP]]
+; CHECK: add sp, sp, #48
 ; CHECK-NEXT: ldp [[CSR1]], [[CSR2]], [sp], #16
 ; CHECK-NEXT: ret
 ;
 ; ENABLE: [[ELSE_LABEL]]: ; %if.else
 ; ENABLE-NEXT: lsl w0, w1, #1
 ; ENABLE-NEXT: ret
 define i32 @callVariadicFunc(i32 %cond, i32 %N) {
 entry:

test/CodeGen/AArch64/fastcc.ll

 ; CHECK: bl func_stack0
 ; CHECK-NOT: sub sp, sp
 
 
 ; CHECK-TAIL: bl func_stack0
 ; CHECK-TAIL-NOT: sub sp, sp
 
   ret void
-; CHECK: mov sp, x29
+; CHECK: add sp, sp, #32
 ; CHECK-NEXT: ldp     x29, x30, [sp], #16
 ; CHECK-NEXT: ret
 
 
-; CHECK-TAIL: mov sp, x29
+; CHECK-TAIL: add sp, sp, #32
 ; CHECK-TAIL-NEXT: ldp     x29, x30, [sp], #16
 ; CHECK-TAIL-NEXT: ret
 }
 
 define fastcc void @func_stack8([8 x i32], i32 %stacked) {
 ; CHECK-LABEL: func_stack8:
 ; CHECK: stp x29, x30, [sp, #-16]!
 ; CHECK: mov x29, sp
; CHECK-TAIL: sub sp, sp, #32
   call fastcc void @func_stack0()
 ; CHECK: bl func_stack0
 ; CHECK-NOT: sub sp, sp
 
 ; CHECK-TAIL: bl func_stack0
 ; CHECK-TAIL-NOT: sub sp, sp
 
   ret void
-; CHECK: mov sp, x29
+; CHECK: add sp, sp, #32
 ; CHECK-NEXT: ldp     x29, x30, [sp], #16
 ; CHECK-NEXT: ret
 
 
-; CHECK-TAIL: mov sp, x29
+; CHECK-TAIL: add sp, sp, #32
 ; CHECK-TAIL-NEXT: ldp     x29, x30, [sp], #16
 ; CHECK-TAIL-NEXT: add     sp, sp, #16
 ; CHECK-TAIL-NEXT: ret
 }
 
 define fastcc void @func_stack32([8 x i32], i128 %stacked0, i128 %stacked1) {
 ; CHECK-LABEL: func_stack32:
 ; CHECK: mov x29, sp

 ; CHECK: bl func_stack0
 ; CHECK-NOT: sub sp, sp
 
 
 ; CHECK-TAIL: bl func_stack0
 ; CHECK-TAIL-NOT: sub sp, sp
 
   ret void
-; CHECK: mov sp, x29
+; CHECK: add sp, sp, #32
 ; CHECK-NEXT: ldp     x29, x30, [sp], #16
 ; CHECK-NEXT: ret
 
-; CHECK-TAIL: mov sp, x29
+; CHECK-TAIL: add sp, sp, #32
 ; CHECK-TAIL-NEXT: ldp     x29, x30, [sp], #16
 ; CHECK-TAIL-NEXT: add     sp, sp, #32
 ; CHECK-TAIL-NEXT: ret
 }
 
 ; Check that arg stack pop is done after callee-save restore when no frame pointer is used.
 define fastcc void @func_stack32_leaf([8 x i32], i128 %stacked0, i128 %stacked1) {
 ; CHECK-LABEL: func_stack32_leaf: