[PATCH] D131485: [AMDGPU] Fix prologue/epilogue markers in .debug_line table for trivial functions

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:974
+  // hits at the beginning of the epilogue.
+  DebugLoc DL = MBBI != MBB.end() ? MBBI->getDebugLoc() : DebugLoc();
 
----------------
RamNalamothu wrote:
> scott.linder wrote:
> > Looking at other targets, I'm not sure how to interpret the case where there are no terminators. Some backends seem to assume there is always a terminator:
> > ```
> > void MSP430FrameLowering::emitEpilogue(MachineFunction &MF,
> >                                        MachineBasicBlock &MBB) const {
> >   ...
> >   DebugLoc DL = MBBI->getDebugLoc();
> > ```
> > 
> > Others also explicitly handle the case where `MBB.empty()`, and also use the `DebugLoc` of the last non-debug instruction instead of just using an empty `DebugLoc()`:
> > ```
> > void CSKYFrameLowering::emitEpilogue(MachineFunction &MF,
> >                                      MachineBasicBlock &MBB) const {
> >   ...
> >   // Get the insert location for the epilogue. If there were no terminators in
> >   // the block, get the last instruction.
> >   MachineBasicBlock::iterator MBBI = MBB.end();
> >   DebugLoc DL;
> >   if (!MBB.empty()) {
> >     MBBI = MBB.getFirstTerminator();
> >     if (MBBI == MBB.end())
> >       MBBI = MBB.getLastNonDebugInstr();
> >     DL = MBBI->getDebugLoc();
> > 
> >     // If this is not a terminator, the actual insert location should be after
> >     // the last instruction.
> >     if (!MBBI->isTerminator())
> >       MBBI = std::next(MBBI);
> >   }
> > ```
> > 
> > There may be other approaches, I didn't review every Target's impl, but I think we should pick one of the existing approaches unless we have a strong reason not to.
> Thanks for catching this Scott.
> 
> What we basically would want to do here is insert epilogue instructions after the last non debug instruction but before the terminator instruction if any. And targets with branch/jump delay slots might have to consider additional scenarios.
> 
> By looking at PEI::calculateSaveRestoreBlocks(), it is possible that some of the restore blocks may not have a terminator. And checking if MBB.empty() is just for efficiency of implementation.
> 
> I think the revised patch should do for our backend.
> 
> I thought I have already submitted this reply.
Maybe use MachineBasicBlock::{r}find{Prev}DebugLoc functions?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131485/new/

https://reviews.llvm.org/D131485