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

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu 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();
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131485