[ARM] Fix PR35481

[chill]

Summary

The issue is when restoring LR when there are no available low registers after popping the callee-saves and we try to find a temporary register before the pop. In the testcase, we are in front of a pop {r7}, however r7 is not in the pop-friendly set (since it's used as a frame pointer), whereas the code assumes that at least one register in the pop is in that set.

This patch allows r7 to be used, regardless of its use as a frame pointer, and also just falls back to using a high register if, for some reason,
we weren't able to find a suitable temporary reg among the popped ones.

Fixes https://bugs.llvm.org/show_bug.cgi?id=35481

[chill]

Ping?

[chill]

Ping?

[efriedma]

lib/Target/ARM/Thumb1FrameLowering.cpp

618 ↗

(On Diff #125969)

If you want exactly the set r0...r7, shouldn't you just set that, rather than getting the set of allocatable registers and adding back specific registers? That would simplify the code a bit because findTemporariesForLR would be guaranteed to succeed.

[chill]

lib/Target/ARM/Thumb1FrameLowering.cpp

618 ↗

(On Diff #125969)

I'd prefer to not depend (or depends less) on which registers are considered allocatable and contain this decision in ARMBaseRegisterInfo::getReservedRegs - to deal with platform specific conventions, possible future options like GCC's -ffixed-reg-N, etc.
That said, I'd like to add back in a similar manner r6/BasePtr.

[chill]

lib/Target/ARM/Thumb1FrameLowering.cpp

618 ↗

(On Diff #125969)

That said, I'd like to add back in a similar manner r6/BasePtr.

I wasn't able to produce a testcase which would take advantage of having a reserved r6 in that set, so I'll
leave it as is.

[efriedma]

LGTM.

lib/Target/ARM/Thumb1FrameLowering.cpp

618 ↗

(On Diff #125969)

Okay.

llvm/trunk/lib/Target/ARM/Thumb1FrameLowering.cpp

bool Thumb1FrameLowering::emitPopSpecialFixUp(MachineBasicBlock &MBB,
   }
 
   // Look for a register that can be directly use in the POP.
   unsigned PopReg = 0;
   // And some temporary register, just in case.
   unsigned TemporaryReg = 0;
   BitVector PopFriendly =
       TRI.getAllocatableSet(MF, TRI.getRegClass(ARM::tGPRRegClassID));
+  // R7 may be used as a frame pointer, hence marked as not generally
+  // allocatable, however there's no reason to not use it as a temporary for
+  // restoring LR.
+  if (STI.useR7AsFramePointer())
+    PopFriendly.set(ARM::R7);
+
   assert(PopFriendly.any() && "No allocatable pop-friendly register?!");
   // Rebuild the GPRs from the high registers because they are removed
   // form the GPR reg class for thumb1.
   BitVector GPRsNoLRSP =
       TRI.getAllocatableSet(MF, TRI.getRegClass(ARM::hGPRRegClassID));
   GPRsNoLRSP |= PopFriendly;
   GPRsNoLRSP.reset(ARM::LR);
   GPRsNoLRSP.reset(ARM::SP);
   GPRsNoLRSP.reset(ARM::PC);
   findTemporariesForLR(GPRsNoLRSP, PopFriendly, UsedRegs, PopReg, TemporaryReg);
 
-  // If we couldn't find a pop-friendly register, restore LR before popping the
-  // other callee-saved registers, so we can use one of them as a temporary.
+  // If we couldn't find a pop-friendly register, try restoring LR before
+  // popping the other callee-saved registers, so we could use one of them as a
+  // temporary.
   bool UseLDRSP = false;
   if (!PopReg && MBBI != MBB.begin()) {
     auto PrevMBBI = MBBI;
     PrevMBBI--;
     if (PrevMBBI->getOpcode() == ARM::tPOP) {
-      MBBI = PrevMBBI;
-      UsedRegs.stepBackward(*MBBI);
+      UsedRegs.stepBackward(*PrevMBBI);
       findTemporariesForLR(GPRsNoLRSP, PopFriendly, UsedRegs, PopReg, TemporaryReg);
+      if (PopReg) {
+        MBBI = PrevMBBI;
         UseLDRSP = true;
       }
     }
+  }
 
   if (!DoIt && !PopReg && !TemporaryReg)
     return false;
 
   assert((PopReg || TemporaryReg) && "Cannot get LR");
 
   if (UseLDRSP) {
     assert(PopReg && "Do not know how to get LR");

llvm/trunk/test/CodeGen/Thumb/PR35481.ll

+; RUN: llc -mtriple thumbv4t-eabi    < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V4T
+; RUN: llc -mtriple armv8m.base-eabi < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V8M
+
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+
+; Function Attrs: nounwind
+define <4 x i32> @f() local_unnamed_addr #0 {
+entry:
+  %call = tail call i32 @h(i32 1)
+  %call1 = tail call <4 x i32> @g(i32 %call, i32 2, i32 3, i32 4)
+  ret <4 x i32> %call1
+; CHECK: ldr r7, [sp, #4]
+; CHECK-NEXT: mov lr, r7
+; CHECK-NEXT: pop {r7}
+; CHECK-NEXT: add sp, #4
+; CHECK-V47: bx lr
+; CHECK-V8M: b g
+}
+
+declare <4 x i32> @g(i32, i32, i32, i32) local_unnamed_addr
+
+declare i32 @h(i32) local_unnamed_addr
+
+attributes #0 = { "disable-tail-calls"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" }