[Mips] Always save RA when disabling frame pointer elimination

[jrtc27]

Summary

This ensures that frame-based unwinding will continue to work when
calling a noreturn function; there is not much use having the caller's
frame pointer saved if you don't also have the caller's program counter.

[jrtc27]

Updated existing tests

[jrtc27]

Stray whitespace

[jrtc27]

Note that the AArch64, ARM, PowerPC (32/64/64le), RISCV (32/64) and SystemZ backends all do this (Sparc gets it by virtue of register windowing so it half counts, and similarly X86 due to the hardware stack push), at least with my C test case (which was reduced to give the IR test case addd here); Mips is currently the odd one out.

[atanasyan]

LGTM. Thanks for the patch.

[draganm]

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

[atanasyan]

In D68542#1697636, @draganm wrote:

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

Agreed. But saving RA has a low cost, might be useful and as far as I can see - gcc saves RA in the same cases.

[jrtc27]

In D68542#1697636, @draganm wrote:

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

You can most of the time, you just have to scan backwards to find the function prologue. Yes, it can break, but unless you have full DWARF info you can't do much better. Both FreeBSD (sys/mips/mips/db_trace.c) and Linux (arch/mips/kernel/process.c) do instruction-based unwinding on MIPS to get a good-enough backtrace on panic, so without this they can end up terminating the backtrace early. In particular, if you want a specific instance of the issue that motivated this patch, on FreeBSD, they have a panic which calls vpanic (much like printf vs vprintf), but due to being marked noreturn, $ra is dead and thus being clobbered by the call doesn't force a save like normal, so *every* panic ends up with a useless backtrace terminating at panic.

[draganm]

In D68542#1697732, @jrtc27 wrote:

In D68542#1697636, @draganm wrote:

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

You can most of the time, you just have to scan backwards to find the function prologue. Yes, it can break, but unless you have full DWARF info you can't do much better. Both FreeBSD (sys/mips/mips/db_trace.c) and Linux (arch/mips/kernel/process.c) do instruction-based unwinding on MIPS to get a good-enough backtrace on panic, so without this they can end up terminating the backtrace early. In particular, if you want a specific instance of the issue that motivated this patch, on FreeBSD, they have a panic which calls vpanic (much like printf vs vprintf), but due to being marked noreturn, $ra is dead and thus being clobbered by the call doesn't force a save like normal, so *every* panic ends up with a useless backtrace terminating at panic.

I see. I haven't loked further but just removing nounwind from callee makes caller save $ra. Thanks for the concrete example. The patch makes sense to me now.

[arichardson]

In D68542#1697732, @jrtc27 wrote:

In D68542#1697636, @draganm wrote:

I don't think you can have frame-pointer based stack unwinding under current Mips ABIs, albeit this might be useful for some stack scan based unwind. Not sure tho.

You can most of the time, you just have to scan backwards to find the function prologue. Yes, it can break, but unless you have full DWARF info you can't do much better. Both FreeBSD (sys/mips/mips/db_trace.c) and Linux (arch/mips/kernel/process.c) do instruction-based unwinding on MIPS to get a good-enough backtrace on panic, so without this they can end up terminating the backtrace early. In particular, if you want a specific instance of the issue that motivated this patch, on FreeBSD, they have a panic which calls vpanic (much like printf vs vprintf), but due to being marked noreturn, $ra is dead and thus being clobbered by the call doesn't force a save like normal, so *every* panic ends up with a useless backtrace terminating at panic.

Would probably be useful to include this example in the commit message.

llvm/lib/Target/Mips/MipsSEFrameLowering.cpp

 
 void MipsSEFrameLowering::determineCalleeSaves(MachineFunction &MF,
                                                BitVector &SavedRegs,
                                                RegScavenger *RS) const {
   TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS);
   const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();
   MipsFunctionInfo *MipsFI = MF.getInfo<MipsFunctionInfo>();
   MipsABIInfo ABI = STI.getABI();
+  unsigned RA = ABI.IsN64() ? Mips::RA_64 : Mips::RA;
   unsigned FP = ABI.GetFramePtr();
   unsigned BP = ABI.IsN64() ? Mips::S7_64 : Mips::S7;
 
-  // Mark $fp as used if function has dedicated frame pointer.
-  if (hasFP(MF))
+  // Mark $ra and $fp as used if function has dedicated frame pointer.
+  if (hasFP(MF)) {
+    setAliasRegs(MF, SavedRegs, RA);
     setAliasRegs(MF, SavedRegs, FP);
+  }
   // Mark $s7 as used if function has dedicated base pointer.
   if (hasBP(MF))
     setAliasRegs(MF, SavedRegs, BP);
 
   // Create spill slots for eh data registers if function calls eh_return.
   if (MipsFI->callsEhReturn())
     MipsFI->createEhDataRegsFI();
 

llvm/test/CodeGen/Mips/no-frame-pointer-elim.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -march=mips64 -relocation-model=static < %s \
+; RUN:   | FileCheck %s --check-prefix STATIC
+; RUN: llc -march=mips64 -relocation-model=pic < %s \
+; RUN:   | FileCheck %s --check-prefix PIC
+
+declare dso_local void @callee() noreturn nounwind
+
+define dso_local void @caller() nounwind "no-frame-pointer-elim-non-leaf" {
+; STATIC-LABEL: caller:
+; STATIC:       # %bb.0: # %entry
+; STATIC-NEXT:    daddiu $sp, $sp, -16
+; STATIC-NEXT:    sd $ra, 8($sp) # 8-byte Folded Spill
+; STATIC-NEXT:    sd $fp, 0($sp) # 8-byte Folded Spill
+; STATIC-NEXT:    move $fp, $sp
+; STATIC-NEXT:    jal callee
+; STATIC-NEXT:    nop
+;
+; PIC-LABEL: caller:
+; PIC:       # %bb.0: # %entry
+; PIC-NEXT:    daddiu $sp, $sp, -32
+; PIC-NEXT:    sd $ra, 24($sp) # 8-byte Folded Spill
+; PIC-NEXT:    sd $fp, 16($sp) # 8-byte Folded Spill
+; PIC-NEXT:    sd $gp, 8($sp) # 8-byte Folded Spill
+; PIC-NEXT:    move $fp, $sp
+; PIC-NEXT:    lui $1, %hi(%neg(%gp_rel(caller)))
+; PIC-NEXT:    daddu $1, $1, $25
+; PIC-NEXT:    daddiu $gp, $1, %lo(%neg(%gp_rel(caller)))
+; PIC-NEXT:    ld $25, %call16(callee)($gp)
+; PIC-NEXT:    .reloc .Ltmp0, R_MIPS_JALR, callee
+; PIC-NEXT:  .Ltmp0:
+; PIC-NEXT:    jalr $25
+; PIC-NEXT:    nop
+entry:
+  tail call void @callee()
+  unreachable
+}