[PATCH 1/2] Consolidate link between registers and spill area

[thopre]

Summary

Context

This patch is part of a patch series to make code for spilling/restoring
callee-saved register more data driven and with less code duplication.
Its purpose is to consolidate the relation between registers and the
spill area they belong into a single function.

Patch description

To decide what registers should be pushed/popped together,
emitPushInst() and emitPopInst() take a function pointer parameter that
return true if a given register should be pushed. ARMBaseRegisterInfo.h
has then one such function for each spill area on the stack.

While whether a register is part of a spill area is mostly static, there
is a bit of variation depending on the target and some per-function
parameter. This lead to quite a bit of code duplication. Having
different functions also forces the caller to be spill area dependent,
eg. it prevents computing the size of each spill areas in a generic way.

This patch introduces an enumeration describing the set of spill areas
and creates a unique function that return to which spill area a given
register belong. It then makes use of it to simplify the code in
ARMFrameLowering in the following way:

• remove duplication of association between registers and spill area when computing size of each spill area
• likewise when emitting frame debug info
• remove logic to distinguish between DPRCS1 and DPRCS2 spill area in many places

All in all, knowledge of link between registers and spill area is now
more centralized and code is smaller: 81 insertions for 125 deletions.

Testing: Ran llvm_regr which shows no regression.

[thopre]

Ping? Would be nice to reduce complexity in this area.

Best regards.

[thopre]

No longer working for Arm. This would need to be rebased first if anybody picks it up.

lib/Target/ARM/ARMBaseRegisterInfo.h

namespace ARMRI {
 
   enum {
     RegPairOdd  = 1,
     RegPairEven = 2
   };
 
 } // end namespace ARMRI
 
-/// isARMArea1Register - Returns true if the register is a low register (r0-r7)
-/// or a stack/pc register that we should push/pop.
-static inline bool isARMArea1Register(unsigned Reg, bool isIOS) {
+/// Sets of register to be pushed/popped together when spilling/restoring
+/// callee-saved registers.
+enum SpillArea {
+  GPRCS1Area,    // GP registers spilled/restored alongside LR/PC
+  GPRCS2Area,    // GP registers spilled/restored separately from LR/PC
+  DPRCS1Area,    // VFP registers without extra SP alignment requirement
+  DPRCS2Area,    // VFP registers requiring 16-byte SP alignment (see
+                 // -align-neon-spills)
+  UnknownCSArea, // Unexpected callee-saved registers
+  CSMaxArea = UnknownCSArea
+};
+
+/// getRegSpillArea - Returns which sets of registers Reg should be
+/// pushed/popped with, as a SpillArea enumerator. SplitGPRSpillArea
+/// controls whether high registers can be pushed together with LR and
+/// NumAlignedDPRCS2Regs the number of registers, starting from D8, in the
+/// DPRCS2Area.
+inline enum SpillArea getRegSpillArea(unsigned Reg, bool SplitGPRSpillArea,
+                                      unsigned NumAlignedDPRCS2Regs) {
   using namespace ARM;
 
   switch (Reg) {
     case R0:  case R1:  case R2:  case R3:
     case R4:  case R5:  case R6:  case R7:
     case LR:  case SP:  case PC:
-      return true;
-    case R8:  case R9:  case R10: case R11: case R12:
-      // For iOS we want r7 and lr to be next to each other.
-      return !isIOS;
-    default:
-      return false;
-  }
-}
-
-static inline bool isARMArea2Register(unsigned Reg, bool isIOS) {
-  using namespace ARM;
-
-  switch (Reg) {
-    case R8: case R9: case R10: case R11: case R12:
-      // iOS has this second area.
-      return isIOS;
+      return GPRCS1Area;
+    case R8:  case R9:  case R10: case R11: case R12:
+      return SplitGPRSpillArea ? GPRCS2Area : GPRCS1Area;
+    case D0:  case D1:  case D2:  case D3:
+    case D4:  case D5:  case D6:  case D7:
+      return DPRCS1Area;
+    case D8:  case D9:  case D10: case D11:
+    case D12: case D13: case D14: case D15:
+    case D16: case D17: case D18: case D19:
+    case D20: case D21: case D22: case D23:
+    case D24: case D25: case D26: case D27:
+    case D28: case D29: case D30: case D31:
+      return (Reg >= D8 + NumAlignedDPRCS2Regs) ? DPRCS1Area : DPRCS2Area;
     default:
-      return false;
-  }
-}
-
-static inline bool isARMArea3Register(unsigned Reg, bool isIOS) {
-  using namespace ARM;
-
-  switch (Reg) {
-    case D15: case D14: case D13: case D12:
-    case D11: case D10: case D9:  case D8:
-    case D7:  case D6:  case D5:  case D4:
-    case D3:  case D2:  case D1:  case D0:
-    case D31: case D30: case D29: case D28:
-    case D27: case D26: case D25: case D24:
-    case D23: case D22: case D21: case D20:
-    case D19: case D18: case D17: case D16:
-      return true;
-    default:
-      return false;
+      assert(false && "getRegSpillArea called with unexpected register");
+      return UnknownCSArea;
   }
 }
 
 static inline bool isCalleeSavedRegister(unsigned Reg,
                                          const MCPhysReg *CSRegs) {
   for (unsigned i = 0; CSRegs[i]; ++i)
     if (Reg == CSRegs[i])
       return true;

lib/Target/ARM/ARMFrameLowering.h

 //===- ARMTargetFrameLowering.h - Define frame lowering for ARM -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_ARM_ARMFRAMELOWERING_H
 #define LLVM_LIB_TARGET_ARM_ARMFRAMELOWERING_H
 
+#include "ARMBaseRegisterInfo.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/TargetFrameLowering.h"
 #include <vector>
 
 namespace llvm {
 
 class ARMSubtarget;
 class CalleeSavedInfo;
public:
   /// Returns true if the target will correctly handle shrink wrapping.
   bool enableShrinkWrapping(const MachineFunction &MF) const override {
     return true;
   }
 
 private:
   void emitPushInst(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
                     const std::vector<CalleeSavedInfo> &CSI, unsigned StmOpc,
-                    unsigned StrOpc, bool NoGap,
-                    bool(*Func)(unsigned, bool), unsigned NumAlignedDPRCS2Regs,
+                    unsigned StrOpc, bool NoGap, enum SpillArea SpillArea,
                     unsigned MIFlags = 0) const;
   void emitPopInst(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
                    std::vector<CalleeSavedInfo> &CSI, unsigned LdmOpc,
                    unsigned LdrOpc, bool isVarArg, bool NoGap,
-                   bool(*Func)(unsigned, bool),
-                   unsigned NumAlignedDPRCS2Regs) const;
+                   enum SpillArea SpillArea) const;
 
   MachineBasicBlock::iterator
   eliminateCallFramePseudoInstr(MachineFunction &MF,
                                 MachineBasicBlock &MBB,
                                 MachineBasicBlock::iterator MI) const override;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_ARM_ARMFRAMELOWERING_H

lib/Target/ARM/ARMFrameLowering.cpp

void ARMFrameLowering::emitPrologue(MachineFunction &MF,
   const ARMBaseInstrInfo &TII = *STI.getInstrInfo();
   assert(!AFI->isThumb1OnlyFunction() &&
          "This emitPrologue does not support Thumb1!");
   bool isARM = !AFI->isThumbFunction();
   unsigned Align = STI.getFrameLowering()->getStackAlignment();
   unsigned ArgRegsSaveSize = AFI->getArgRegsSaveSize();
   unsigned NumBytes = MFI.getStackSize();
   const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();
+  bool SplitFramePushPop = STI.splitFramePushPop(MF);
+  unsigned NumAlignedDPRCS2Regs = AFI->getNumAlignedDPRCS2Regs();
 
   // Debug location must be unknown since the first debug location is used
   // to determine the end of the prologue.
   DebugLoc dl;
   
   unsigned FramePtr = RegInfo->getFrameRegister(MF);
 
   // Determine the sizes of each callee-save spill areas and record which frame
if (!AFI->hasStackFrame() &&
     DefCFAOffsetCandidates.emitDefCFAOffsets(MBB, dl, TII, HasFP);
     return;
   }
 
   // Determine spill area sizes.
   for (unsigned i = 0, e = CSI.size(); i != e; ++i) {
     unsigned Reg = CSI[i].getReg();
     int FI = CSI[i].getFrameIdx();
-    switch (Reg) {
-    case ARM::R8:
-    case ARM::R9:
-    case ARM::R10:
-    case ARM::R11:
-    case ARM::R12:
-      if (STI.splitFramePushPop(MF)) {
-        GPRCS2Size += 4;
-        break;
-      }
-      LLVM_FALLTHROUGH;
-    case ARM::R0:
-    case ARM::R1:
-    case ARM::R2:
-    case ARM::R3:
-    case ARM::R4:
-    case ARM::R5:
-    case ARM::R6:
-    case ARM::R7:
-    case ARM::LR:
+    switch (getRegSpillArea(Reg, SplitFramePushPop, NumAlignedDPRCS2Regs)) {
+    case GPRCS1Area:
       if (Reg == FramePtr)
         FramePtrSpillFI = FI;
       GPRCS1Size += 4;
       break;
-    default:
-      // This is a DPR. Exclude the aligned DPRCS2 spills.
+    case GPRCS2Area:
+      GPRCS2Size += 4;
+      break;
+    case DPRCS1Area:
+      DPRCSSize += 8;
+      LLVM_FALLTHROUGH;
+    case DPRCS2Area:
       if (Reg == ARM::D8)
         D8SpillFI = FI;
-      if (Reg < ARM::D8 || Reg >= ARM::D8 + AFI->getNumAlignedDPRCS2Regs())
-        DPRCSSize += 8;
+      break;
+    default:
+      assert(false && "Unknown spill size");
     }
   }
 
   // Move past area 1.
   MachineBasicBlock::iterator LastPush = MBB.end(), GPRCS1Push, GPRCS2Push;
   if (GPRCS1Size > 0) {
     GPRCS1Push = LastPush = MBBI++;
     DefCFAOffsetCandidates.addInst(LastPush, GPRCS1Size, true);
void ARMFrameLowering::emitPrologue(MachineFunction &MF,
   // the necessary DWARF cf instructions to describe the situation. Start by
   // recording where each register ended up:
   if (GPRCS1Size > 0) {
     MachineBasicBlock::iterator Pos = std::next(GPRCS1Push);
     int CFIIndex;
     for (const auto &Entry : CSI) {
       unsigned Reg = Entry.getReg();
       int FI = Entry.getFrameIdx();
-      switch (Reg) {
-      case ARM::R8:
-      case ARM::R9:
-      case ARM::R10:
-      case ARM::R11:
-      case ARM::R12:
-        if (STI.splitFramePushPop(MF))
-          break;
-        LLVM_FALLTHROUGH;
-      case ARM::R0:
-      case ARM::R1:
-      case ARM::R2:
-      case ARM::R3:
-      case ARM::R4:
-      case ARM::R5:
-      case ARM::R6:
-      case ARM::R7:
-      case ARM::LR:
+      if (getRegSpillArea(Reg, SplitFramePushPop, NumAlignedDPRCS2Regs) ==
+          GPRCS1Area) {
         CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
             nullptr, MRI->getDwarfRegNum(Reg, true), MFI.getObjectOffset(FI)));
         BuildMI(MBB, Pos, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
             .addCFIIndex(CFIIndex)
             .setMIFlags(MachineInstr::FrameSetup);
-        break;
       }
     }
   }
 
   if (GPRCS2Size > 0) {
     MachineBasicBlock::iterator Pos = std::next(GPRCS2Push);
     for (const auto &Entry : CSI) {
       unsigned Reg = Entry.getReg();
       int FI = Entry.getFrameIdx();
-      switch (Reg) {
-      case ARM::R8:
-      case ARM::R9:
-      case ARM::R10:
-      case ARM::R11:
-      case ARM::R12:
-        if (STI.splitFramePushPop(MF)) {
-          unsigned DwarfReg =  MRI->getDwarfRegNum(Reg, true);
-          unsigned Offset = MFI.getObjectOffset(FI);
-          unsigned CFIIndex = MF.addFrameInst(
-              MCCFIInstruction::createOffset(nullptr, DwarfReg, Offset));
-          BuildMI(MBB, Pos, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
-              .addCFIIndex(CFIIndex)
-              .setMIFlags(MachineInstr::FrameSetup);
-        }
-        break;
-      }
+      if (getRegSpillArea(Reg, SplitFramePushPop, NumAlignedDPRCS2Regs) ==
+          GPRCS2Area) {
+        unsigned DwarfReg = MRI->getDwarfRegNum(Reg, true);
     }
   }
 
   if (DPRCSSize > 0) {
     // Since vpush register list cannot have gaps, there may be multiple vpush
     // instructions in the prologue.
     MachineBasicBlock::iterator Pos = std::next(LastPush);
     for (const auto &Entry : CSI) {
       unsigned Reg = Entry.getReg();
       int FI = Entry.getFrameIdx();
-      if ((Reg >= ARM::D0 && Reg <= ARM::D31) &&
-          (Reg < ARM::D8 || Reg >= ARM::D8 + AFI->getNumAlignedDPRCS2Regs())) {
+      if (getRegSpillArea(Reg, SplitFramePushPop, NumAlignedDPRCS2Regs) ==
+          DPRCS1Area) {
         unsigned DwarfReg = MRI->getDwarfRegNum(Reg, true);
         unsigned Offset = MFI.getObjectOffset(FI);
         unsigned CFIIndex = MF.addFrameInst(
             MCCFIInstruction::createOffset(nullptr, DwarfReg, Offset));
         BuildMI(MBB, Pos, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
             .addCFIIndex(CFIIndex)
             .setMIFlags(MachineInstr::FrameSetup);
       }
if (RegInfo->hasBasePointer(MF))
     FrameReg = RegInfo->getBaseRegister();
   return Offset;
 }
 
 void ARMFrameLowering::emitPushInst(MachineBasicBlock &MBB,
                                     MachineBasicBlock::iterator MI,
                                     const std::vector<CalleeSavedInfo> &CSI,
                                     unsigned StmOpc, unsigned StrOpc,
-                                    bool NoGap,
-                                    bool(*Func)(unsigned, bool),
-                                    unsigned NumAlignedDPRCS2Regs,
+                                    bool NoGap, enum SpillArea SpillArea,
                                     unsigned MIFlags) const {
   MachineFunction &MF = *MBB.getParent();
+  ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   const TargetRegisterInfo &TRI = *STI.getRegisterInfo();
+  bool SplitFramePushPop = STI.splitFramePushPop(MF);
+  unsigned NumAlignedDPRCS2Regs = AFI->getNumAlignedDPRCS2Regs();
 
   DebugLoc DL;
 
   using RegAndKill = std::pair<unsigned, bool>;
 
   SmallVector<RegAndKill, 4> Regs;
   unsigned i = CSI.size();
   while (i != 0) {
     unsigned LastReg = 0;
     for (; i != 0; --i) {
       unsigned Reg = CSI[i-1].getReg();
-      if (!(Func)(Reg, STI.splitFramePushPop(MF))) continue;
-
-      // D-registers in the aligned area DPRCS2 are NOT spilled here.
-      if (Reg >= ARM::D8 && Reg < ARM::D8 + NumAlignedDPRCS2Regs)
+      if (getRegSpillArea(Reg, SplitFramePushPop, NumAlignedDPRCS2Regs) !=
+          SpillArea)
         continue;
 
       const MachineRegisterInfo &MRI = MF.getRegInfo();
       bool isLiveIn = MRI.isLiveIn(Reg);
       if (!isLiveIn && !MRI.isReserved(Reg))
         MBB.addLiveIn(Reg);
       // If NoGap is true, push consecutive registers and then leave the rest
       // for other instructions. e.g.
void ARMFrameLowering::emitPushInst(MachineBasicBlock &MBB,
   }
 }
 
 void ARMFrameLowering::emitPopInst(MachineBasicBlock &MBB,
                                    MachineBasicBlock::iterator MI,
                                    std::vector<CalleeSavedInfo> &CSI,
                                    unsigned LdmOpc, unsigned LdrOpc,
                                    bool isVarArg, bool NoGap,
-                                   bool(*Func)(unsigned, bool),
-                                   unsigned NumAlignedDPRCS2Regs) const {
+                                   enum SpillArea SpillArea) const {
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   const TargetRegisterInfo &TRI = *STI.getRegisterInfo();
   ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
+  bool SplitFramePushPop = STI.splitFramePushPop(MF);
+  unsigned NumAlignedDPRCS2Regs = AFI->getNumAlignedDPRCS2Regs();
   DebugLoc DL;
   bool isTailCall = false;
   bool isInterrupt = false;
   bool isTrap = false;
   if (MBB.end() != MI) {
     DL = MI->getDebugLoc();
     unsigned RetOpcode = MI->getOpcode();
     isTailCall = (RetOpcode == ARM::TCRETURNdi || RetOpcode == ARM::TCRETURNri);
     isInterrupt =
         RetOpcode == ARM::SUBS_PC_LR || RetOpcode == ARM::t2SUBS_PC_LR;
     isTrap =
         RetOpcode == ARM::TRAP || RetOpcode == ARM::TRAPNaCl ||
         RetOpcode == ARM::tTRAP;
   }
 
   SmallVector<unsigned, 4> Regs;
   unsigned i = CSI.size();
   while (i != 0) {
     unsigned LastReg = 0;
     bool DeleteRet = false;
     for (; i != 0; --i) {
       CalleeSavedInfo &Info = CSI[i-1];
       unsigned Reg = Info.getReg();
-      if (!(Func)(Reg, STI.splitFramePushPop(MF))) continue;
-
-      // The aligned reloads from area DPRCS2 are not inserted here.
-      if (Reg >= ARM::D8 && Reg < ARM::D8 + NumAlignedDPRCS2Regs)
+      if (getRegSpillArea(Reg, SplitFramePushPop, NumAlignedDPRCS2Regs) !=
+          SpillArea)
         continue;
 
       if (Reg == ARM::LR && !isTailCall && !isVarArg && !isInterrupt &&
           !isTrap && STI.hasV5TOps()) {
         if (MBB.succ_empty()) {
           Reg = ARM::PC;
           DeleteRet = true;
           LdmOpc = AFI->isThumbFunction() ? ARM::t2LDMIA_RET : ARM::LDMIA_RET;
bool ARMFrameLowering::spillCalleeSavedRegisters(MachineBasicBlock &MBB,
   MachineFunction &MF = *MBB.getParent();
   ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
 
   unsigned PushOpc = AFI->isThumbFunction() ? ARM::t2STMDB_UPD : ARM::STMDB_UPD;
   unsigned PushOneOpc = AFI->isThumbFunction() ?
     ARM::t2STR_PRE : ARM::STR_PRE_IMM;
   unsigned FltOpc = ARM::VSTMDDB_UPD;
   unsigned NumAlignedDPRCS2Regs = AFI->getNumAlignedDPRCS2Regs();
-  emitPushInst(MBB, MI, CSI, PushOpc, PushOneOpc, false, &isARMArea1Register, 0,
+  emitPushInst(MBB, MI, CSI, PushOpc, PushOneOpc, false, GPRCS1Area,
+               MachineInstr::FrameSetup);
+  emitPushInst(MBB, MI, CSI, PushOpc, PushOneOpc, false, GPRCS2Area,
                MachineInstr::FrameSetup);
-  emitPushInst(MBB, MI, CSI, PushOpc, PushOneOpc, false, &isARMArea2Register, 0,
+  emitPushInst(MBB, MI, CSI, FltOpc, 0, true, DPRCS1Area,
                MachineInstr::FrameSetup);
-  emitPushInst(MBB, MI, CSI, FltOpc, 0, true, &isARMArea3Register,
-               NumAlignedDPRCS2Regs, MachineInstr::FrameSetup);
 
   // The code above does not insert spill code for the aligned DPRCS2 registers.
   // The stack realignment code will be inserted between the push instructions
   // and these spills.
   if (NumAlignedDPRCS2Regs)
     emitAlignedDPRCS2Spills(MBB, MI, NumAlignedDPRCS2Regs, CSI, TRI);
 
   return true;
bool ARMFrameLowering::restoreCalleeSavedRegisters(MachineBasicBlock &MBB,
   // The emitPopInst calls below do not insert reloads for the aligned DPRCS2
   // registers. Do that here instead.
   if (NumAlignedDPRCS2Regs)
     emitAlignedDPRCS2Restores(MBB, MI, NumAlignedDPRCS2Regs, CSI, TRI);
 
   unsigned PopOpc = AFI->isThumbFunction() ? ARM::t2LDMIA_UPD : ARM::LDMIA_UPD;
   unsigned LdrOpc = AFI->isThumbFunction() ? ARM::t2LDR_POST :ARM::LDR_POST_IMM;
   unsigned FltOpc = ARM::VLDMDIA_UPD;
-  emitPopInst(MBB, MI, CSI, FltOpc, 0, isVarArg, true, &isARMArea3Register,
-              NumAlignedDPRCS2Regs);
-  emitPopInst(MBB, MI, CSI, PopOpc, LdrOpc, isVarArg, false,
-              &isARMArea2Register, 0);
-  emitPopInst(MBB, MI, CSI, PopOpc, LdrOpc, isVarArg, false,
-              &isARMArea1Register, 0);
+  emitPopInst(MBB, MI, CSI, FltOpc, 0, isVarArg, true, DPRCS1Area);
+  emitPopInst(MBB, MI, CSI, PopOpc, LdrOpc, isVarArg, false, GPRCS2Area);
+  emitPopInst(MBB, MI, CSI, PopOpc, LdrOpc, isVarArg, false, GPRCS1Area);
 
   return true;
 }
 
 // FIXME: Make generic?
 static unsigned GetFunctionSizeInBytes(const MachineFunction &MF,
                                        const ARMBaseInstrInfo &TII) {
   unsigned FnSize = 0;