[PATCH] ARM: fix Oz sp-adjust folding to respect callee-saved registers

[Anton Korobeynikov]

Should this eventually be applied to release branch?

On Thu, Nov 28, 2013 at 3:21 PM, Tim Northover <t.p.northover at gmail.com> wrote:
> Oops. It seems the original commit was almost guaranteed to break on any code it applied to since it clobbered callee-saved registers with gleeful abandon.
>
> In order to still be useful, a slight refactoring also allows registers to be skipped from the list if the instruction allows. This permits useful lists like {r0, r1, r2, r3, pc} to be used.
>
> This should fix http://llvm.org/bugs/show_bug.cgi?id=18081.
>
> http://llvm-reviews.chandlerc.com/D2285
>
> Files:
>   lib/Target/ARM/ARMBaseInstrInfo.cpp
>   lib/Target/ARM/ARMBaseRegisterInfo.h
>   lib/Target/ARM/ARMFrameLowering.cpp
>   lib/Target/ARM/Thumb1FrameLowering.cpp
>   test/CodeGen/ARM/fold-stack-adjust.ll
>
> Index: lib/Target/ARM/ARMBaseInstrInfo.cpp
> ===================================================================
> --- lib/Target/ARM/ARMBaseInstrInfo.cpp
> +++ lib/Target/ARM/ARMBaseInstrInfo.cpp
> @@ -1909,29 +1909,40 @@
>
>    MachineBasicBlock *MBB = MI->getParent();
>    const TargetRegisterInfo *TRI = MF.getRegInfo().getTargetRegisterInfo();
> +  const MCPhysReg *CSRegs = TRI->getCalleeSavedRegs(&MF);
>
>    // Now try to find enough space in the reglist to allocate NumBytes.
>    for (unsigned CurReg = FirstReg - 1; CurReg >= RD0Reg && RegsNeeded;
> -       --CurReg, --RegsNeeded) {
> +       --CurReg) {
>      if (!IsPop) {
>        // Pushing any register is completely harmless, mark the
>        // register involved as undef since we don't care about it in
>        // the slightest.
>        RegList.push_back(MachineOperand::CreateReg(CurReg, false, false,
>                                                    false, false, true));
> +      --RegsNeeded;
>        continue;
>      }
>
> -    // However, we can only pop an extra register if it's not live. Otherwise we
> -    // might clobber a return value register. We assume that once we find a live
> -    // return register all lower ones will be too so there's no use proceeding.
> -    if (MBB->computeRegisterLiveness(TRI, CurReg, MI) !=
> -        MachineBasicBlock::LQR_Dead)
> -      return false;
> +    // However, we can only pop an extra register if it's not live. For
> +    // registers live within the function we might clobber a return value
> +    // register; the other way a register can be live here is if it's
> +    // callee-saved.
> +    if (isCalleeSavedRegister(CurReg, CSRegs) ||
> +        MBB->computeRegisterLiveness(TRI, CurReg, MI) !=
> +            MachineBasicBlock::LQR_Dead) {
> +      // VFP pops don't allow holes in the register list, so any skip is fatal
> +      // for our transformation. GPR pops do, so we should just keep looking.
> +      if (IsVFPPushPop)
> +        return false;
> +      else
> +        continue;
> +    }
>
>      // Mark the unimportant registers as <def,dead> in the POP.
>      RegList.push_back(MachineOperand::CreateReg(CurReg, true, false, false,
>                                                  true));
> +    --RegsNeeded;
>    }
>
>    if (RegsNeeded > 0)
> Index: lib/Target/ARM/ARMBaseRegisterInfo.h
> ===================================================================
> --- lib/Target/ARM/ARMBaseRegisterInfo.h
> +++ lib/Target/ARM/ARMBaseRegisterInfo.h
> @@ -72,6 +72,14 @@
>    }
>  }
>
> +static inline bool isCalleeSavedRegister(unsigned Reg,
> +                                         const MCPhysReg *CSRegs) {
> +  for (unsigned i = 0; CSRegs[i]; ++i)
> +    if (Reg == CSRegs[i])
> +      return true;
> +  return false;
> +}
> +
>  class ARMBaseRegisterInfo : public ARMGenRegisterInfo {
>  protected:
>    const ARMSubtarget &STI;
> Index: lib/Target/ARM/ARMFrameLowering.cpp
> ===================================================================
> --- lib/Target/ARM/ARMFrameLowering.cpp
> +++ lib/Target/ARM/ARMFrameLowering.cpp
> @@ -82,13 +82,6 @@
>    return hasReservedCallFrame(MF) || MF.getFrameInfo()->hasVarSizedObjects();
>  }
>
> -static bool isCalleeSavedRegister(unsigned Reg, const uint16_t *CSRegs) {
> -  for (unsigned i = 0; CSRegs[i]; ++i)
> -    if (Reg == CSRegs[i])
> -      return true;
> -  return false;
> -}
> -
>  static bool isCSRestore(MachineInstr *MI,
>                          const ARMBaseInstrInfo &TII,
>                          const uint16_t *CSRegs) {
> Index: lib/Target/ARM/Thumb1FrameLowering.cpp
> ===================================================================
> --- lib/Target/ARM/Thumb1FrameLowering.cpp
> +++ lib/Target/ARM/Thumb1FrameLowering.cpp
> @@ -215,13 +215,6 @@
>      AFI->setShouldRestoreSPFromFP(true);
>  }
>
> -static bool isCalleeSavedRegister(unsigned Reg, const uint16_t *CSRegs) {
> -  for (unsigned i = 0; CSRegs[i]; ++i)
> -    if (Reg == CSRegs[i])
> -      return true;
> -  return false;
> -}
> -
>  static bool isCSRestore(MachineInstr *MI, const uint16_t *CSRegs) {
>    if (MI->getOpcode() == ARM::tLDRspi &&
>        MI->getOperand(1).isFI() &&
> Index: test/CodeGen/ARM/fold-stack-adjust.ll
> ===================================================================
> --- test/CodeGen/ARM/fold-stack-adjust.ll
> +++ test/CodeGen/ARM/fold-stack-adjust.ll
> @@ -15,15 +15,15 @@
>  ; CHECK-NOT: sub sp, sp,
>  ; ...
>  ; CHECK-NOT: add sp, sp,
> -; CHECK: pop.w {r7, r8, r9, r10, r11, pc}
> +; CHECK: pop.w {r0, r1, r2, r3, r11, pc}
>
>  ; CHECK-T1-LABEL: check_simple:
>  ; CHECK-T1: push {r3, r4, r5, r6, r7, lr}
>  ; CHECK-T1: add r7, sp, #16
>  ; CHECK-T1-NOT: sub sp, sp,
>  ; ...
>  ; CHECK-T1-NOT: add sp, sp,
> -; CHECK-T1: pop {r3, r4, r5, r6, r7, pc}
> +; CHECK-T1: pop {r0, r1, r2, r3, r7, pc}
>
>    ; iOS always has a frame pointer and messing with the push affects
>    ; how it's set in the prologue. Make sure we get that right.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



-- 
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University