[AVR] Remove redundant dynalloca SP save/restore pass

I think this pass was previously used under the assumption that most
functions would not need a frame pointer and it would be more efficient
to store the old stack pointer in a regular register pair.

Unfortunately, right now we're forced to always reserve the Y register
as a frame pointer: whether or not this is needed is only known after
regsiter allocation at which point it doesn't make sense anymore to mark
it as non-reserved. Therefore, it makes sense to use the Y register to
store the old stack pointer in functions with dynamic allocas (with a
variable size or not in the entry block). Knowing this can make the code
around dynamic allocas a lot simpler: simply save/restore the frame
pointer.

This is especially relevant in functions that have a frame pointer
anyway (for example, because they have stack spills). The stack restore
in the epilogue will implicitly restore the old stack pointer, so there
is no need to store the old stack pointer separately. It even reduces
register pressure as a side effect.

Differential Revision: https://reviews.llvm.org/D97815

llvm/lib/Target/AVR/AVR.h

@@ -28,7 +28,6 @@ FunctionPass *createAVRISelDag(AVRTargetMachine &TM,
 FunctionPass *createAVRExpandPseudoPass();
 FunctionPass *createAVRFrameAnalyzerPass();
 FunctionPass *createAVRRelaxMemPass();
-FunctionPass *createAVRDynAllocaSRPass();
 FunctionPass *createAVRBranchSelectionPass();
 
 void initializeAVRShiftExpandPass(PassRegistry &);

llvm/lib/Target/AVR/AVRFrameLowering.cpp

@@ -176,7 +176,7 @@ void AVRFrameLowering::emitEpilogue(MachineFunction &MF,
   const AVRInstrInfo &TII = *STI.getInstrInfo();
 
   // Early exit if there is no need to restore the frame pointer.
-  if (!FrameSize) {
+  if (!FrameSize && !MF.getFrameInfo().hasVarSizedObjects()) {
     restoreStatusRegister(MF, MBB);
     return;
   }
@@ -193,23 +193,25 @@ void AVRFrameLowering::emitEpilogue(MachineFunction &MF,
     --MBBI;
   }
 
-  unsigned Opcode;
+  if (FrameSize) {
+    unsigned Opcode;
 
-  // Select the optimal opcode depending on how big it is.
-  if (isUInt<6>(FrameSize)) {
-    Opcode = AVR::ADIWRdK;
-  } else {
-    Opcode = AVR::SUBIWRdK;
-    FrameSize = -FrameSize;
+    // Select the optimal opcode depending on how big it is.
+    if (isUInt<6>(FrameSize)) {
+      Opcode = AVR::ADIWRdK;
+    } else {
+      Opcode = AVR::SUBIWRdK;
+      FrameSize = -FrameSize;
+    }
+
+    // Restore the frame pointer by doing FP += <size>.
+    MachineInstr *MI = BuildMI(MBB, MBBI, DL, TII.get(Opcode), AVR::R29R28)
+                          .addReg(AVR::R29R28, RegState::Kill)
+                          .addImm(FrameSize);
+    // The SREG implicit def is dead.
+    MI->getOperand(3).setIsDead();
   }
 
-  // Restore the frame pointer by doing FP += <size>.
-  MachineInstr *MI = BuildMI(MBB, MBBI, DL, TII.get(Opcode), AVR::R29R28)
-                         .addReg(AVR::R29R28, RegState::Kill)
-                         .addImm(FrameSize);
-  // The SREG implicit def is dead.
-  MI->getOperand(3).setIsDead();
-
   // Write back R29R28 to SP and temporarily disable interrupts.
   BuildMI(MBB, MBBI, DL, TII.get(AVR::SPWRITE), AVR::SP)
       .addReg(AVR::R29R28, RegState::Kill);
@@ -230,7 +232,8 @@ bool AVRFrameLowering::hasFP(const MachineFunction &MF) const {
   const AVRMachineFunctionInfo *FuncInfo = MF.getInfo<AVRMachineFunctionInfo>();
 
   return (FuncInfo->getHasSpills() || FuncInfo->getHasAllocas() ||
-          FuncInfo->getHasStackArgs());
+          FuncInfo->getHasStackArgs() ||
+          MF.getFrameInfo().hasVarSizedObjects());
 }
 
 bool AVRFrameLowering::spillCalleeSavedRegisters(
@@ -480,56 +483,4 @@ char AVRFrameAnalyzer::ID = 0;
 /// Creates instance of the frame analyzer pass.
 FunctionPass *createAVRFrameAnalyzerPass() { return new AVRFrameAnalyzer(); }
 
-/// Create the Dynalloca Stack Pointer Save/Restore pass.
-/// Insert a copy of SP before allocating the dynamic stack memory and restore
-/// it in function exit to restore the original SP state. This avoids the need
-/// of reserving a register pair for a frame pointer.
-struct AVRDynAllocaSR : public MachineFunctionPass {
-  static char ID;
-  AVRDynAllocaSR() : MachineFunctionPass(ID) {}
-
-  bool runOnMachineFunction(MachineFunction &MF) override {
-    // Early exit when there are no variable sized objects in the function.
-    if (!MF.getFrameInfo().hasVarSizedObjects()) {
-      return false;
-    }
-
-    const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
-    const TargetInstrInfo &TII = *STI.getInstrInfo();
-    MachineBasicBlock &EntryMBB = MF.front();
-    MachineBasicBlock::iterator MBBI = EntryMBB.begin();
-    DebugLoc DL = EntryMBB.findDebugLoc(MBBI);
-
-    Register SPCopy =
-        MF.getRegInfo().createVirtualRegister(&AVR::DREGSRegClass);
-
-    // Create a copy of SP in function entry before any dynallocas are
-    // inserted.
-    BuildMI(EntryMBB, MBBI, DL, TII.get(AVR::COPY), SPCopy).addReg(AVR::SP);
-
-    // Restore SP in all exit basic blocks.
-    for (MachineBasicBlock &MBB : MF) {
-      // If last instruction is a return instruction, add a restore copy.
-      if (!MBB.empty() && MBB.back().isReturn()) {
-        MBBI = MBB.getLastNonDebugInstr();
-        DL = MBBI->getDebugLoc();
-        BuildMI(MBB, MBBI, DL, TII.get(AVR::COPY), AVR::SP)
-            .addReg(SPCopy, RegState::Kill);
-      }
-    }
-
-    return true;
-  }
-
-  StringRef getPassName() const override {
-    return "AVR dynalloca stack pointer save/restore";
-  }
-};
-
-char AVRDynAllocaSR::ID = 0;
-
-/// createAVRDynAllocaSRPass - returns an instance of the dynalloca stack
-/// pointer save/restore pass.
-FunctionPass *createAVRDynAllocaSRPass() { return new AVRDynAllocaSR(); }
-
 } // end of namespace llvm

llvm/lib/Target/AVR/AVRTargetMachine.cpp

@@ -70,7 +70,6 @@ public:
   bool addInstSelector() override;
   void addPreSched2() override;
   void addPreEmitPass() override;
-  void addPreRegAlloc() override;
 };
 } // namespace
 
@@ -118,11 +117,6 @@ bool AVRPassConfig::addInstSelector() {
   return false;
 }
 
-void AVRPassConfig::addPreRegAlloc() {
-  // Create the dynalloc SP save/restore pass to handle variable sized allocas.
-  addPass(createAVRDynAllocaSRPass());
-}
-
 void AVRPassConfig::addPreSched2() {
   addPass(createAVRRelaxMemPass());
   addPass(createAVRExpandPseudoPass());

llvm/test/CodeGen/AVR/dynalloca.ll

@@ -4,10 +4,10 @@ declare void @foo(i16*, i16*, i8*)
 
 define void @test1(i16 %x) {
 ; CHECK-LABEL: test1:
+; Frame setup, with frame pointer
+; CHECK: in r28, 61
+; CHECK: in r29, 62
 ; CHECK: out 61, r28
-; SP copy
-; CHECK-NEXT: in [[SPCOPY1:r[0-9]+]], 61
-; CHECK-NEXT: in [[SPCOPY2:r[0-9]+]], 62
 ; allocate first dynalloca
 ; CHECK: in {{.*}}, 61
 ; CHECK: in {{.*}}, 62
@@ -26,9 +26,9 @@ define void @test1(i16 %x) {
 ; Test SP restore
 ; CHECK: in r0, 63
 ; CHECK-NEXT: cli
-; CHECK-NEXT: out 62, [[SPCOPY2]]
+; CHECK-NEXT: out 62, r29
 ; CHECK-NEXT: out 63, r0
-; CHECK-NEXT: out 61, [[SPCOPY1]]
+; CHECK-NEXT: out 61, r28
   %a = alloca [8 x i16]
   %vla = alloca i16, i16 %x
   %add = shl nsw i16 %x, 1
@@ -51,8 +51,8 @@ declare void @foo2(i16*, i64, i64, i64)
 ; after the call frame is restored and not before.
 define void @dynalloca2(i16 %x) {
 ; CHECK-LABEL: dynalloca2:
-; CHECK: in [[SPCOPY1:r[0-9]+]], 61
-; CHECK: in [[SPCOPY2:r[0-9]+]], 62
+; CHECK: in r28, 61
+; CHECK: in r29, 62
 ; Allocate stack space for call
 ; CHECK: in {{.*}}, 61
 ; CHECK: in {{.*}}, 62
@@ -87,10 +87,40 @@ define void @dynalloca2(i16 %x) {
 ; SP restore
 ; CHECK: in r0, 63
 ; CHECK-NEXT: cli
-; CHECK-NEXT: out 62, r7
+; CHECK-NEXT: out 62, r29
 ; CHECK-NEXT: out 63, r0
-; CHECK-NEXT: out 61, r6
+; CHECK-NEXT: out 61, r28
   %vla = alloca i16, i16 %x
   call void @foo2(i16* %vla, i64 0, i64 0, i64 0)
   ret void
 }
+
+; Test a function with a variable sized object but without any other need for a
+; frame pointer.
+; Allocas that are not placed in the entry block are considered variable sized
+; (they could be in a loop).
+define void @dynalloca3() {
+; CHECK-LABEL: dynalloca3:
+; Read frame pointer
+; CHECK:      in r28, 61
+; CHECK-NEXT: in r29, 62
+; Allocate memory for the alloca
+; CHECK-NEXT: in r24, 61
+; CHECK-NEXT: in r25, 62
+; CHECK-NEXT: sbiw r24, 8
+; CHECK-NEXT: in r0, 63
+; CHECK-NEXT: cli
+; CHECK-NEXT: out 62, r25
+; CHECK-NEXT: out 63, r0
+; CHECK-NEXT: out 61, r24
+; Restore frame pointer
+; CHECK-NEXT: in r0, 63
+; CHECK-NEXT: cli
+; CHECK-NEXT: out 62, r29
+; CHECK-NEXT: out 63, r0
+; CHECK-NEXT: out 61, r28
+  br label %1
+1:
+  %a = alloca i64
+  ret void
+}