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

[RamNalamothu]

Summary

All the prologue instructions should have unknown source location
co-ordinates while the epilogue instructions should have source
location of last non-debug instruction after which epilogue
instructions are insrted.

This ensures the prologue/epilogue markers are generated correctly
in the line table.

Changes are brought in from the downstream CFI patches.

[scott.linder]

One nit, but the rest LGTM

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

976

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.

[RamNalamothu]

Address review comments.

[scott.linder]

LGTM

Maybe in the future the various versions of this could be merged, but as it stands there seems to be essentially N targets which do this N different ways, which is a problem we can't solve here.

[RamNalamothu]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

976

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.

[foad]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

976

Maybe use MachineBasicBlock::{r}find{Prev}DebugLoc functions?

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

static void initLiveRegs(LivePhysRegs &LiveRegs, const SIRegisterInfo &TRI,
   }
 }
 
 // Activate all lanes, returns saved exec.
 static Register buildScratchExecCopy(LivePhysRegs &LiveRegs,
                                      MachineFunction &MF,
                                      MachineBasicBlock &MBB,
                                      MachineBasicBlock::iterator MBBI,
-                                     bool IsProlog) {
+                                     const DebugLoc &DL, bool IsProlog) {
   Register ScratchExecCopy;
   MachineRegisterInfo &MRI = MF.getRegInfo();
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
   const SIInstrInfo *TII = ST.getInstrInfo();
   const SIRegisterInfo &TRI = TII->getRegisterInfo();
   SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
-  DebugLoc DL;
 
   initLiveRegs(LiveRegs, TRI, FuncInfo, MF, MBB, MBBI, IsProlog);
 
   ScratchExecCopy = findScratchNonCalleeSaveRegister(
       MRI, LiveRegs, *TRI.getWaveMaskRegClass());
   if (!ScratchExecCopy)
     report_fatal_error("failed to find free scratch register");
 
void SIFrameLowering::emitPrologue(MachineFunction &MF,
 
   Register StackPtrReg = FuncInfo->getStackPtrOffsetReg();
   Register FramePtrReg = FuncInfo->getFrameOffsetReg();
   Register BasePtrReg =
       TRI.hasBasePointer(MF) ? TRI.getBaseRegister() : Register();
   LivePhysRegs LiveRegs;
 
   MachineBasicBlock::iterator MBBI = MBB.begin();
+  // DebugLoc must be unknown since the first instruction with DebugLoc is used
+  // to determine the end of the prologue.
   DebugLoc DL;
 
   bool HasFP = false;
   bool HasBP = false;
   uint32_t NumBytes = MFI.getStackSize();
   uint32_t RoundedSize = NumBytes;
   // To avoid clobbering VGPRs in lanes that weren't active on function entry,
   // turn on all lanes before doing the spill to memory.
   Register ScratchExecCopy;
 
   Optional<int> FPSaveIndex = FuncInfo->FramePointerSaveIndex;
   Optional<int> BPSaveIndex = FuncInfo->BasePointerSaveIndex;
 
   // VGPRs used for SGPR->VGPR spills
   for (const SIMachineFunctionInfo::SGPRSpillVGPR &Reg :
        FuncInfo->getSGPRSpillVGPRs()) {
     if (!Reg.FI)
       continue;
 
     if (!ScratchExecCopy)
-      ScratchExecCopy = buildScratchExecCopy(LiveRegs, MF, MBB, MBBI,
+      ScratchExecCopy = buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, DL,
                                              /*IsProlog*/ true);
 
     buildPrologSpill(ST, TRI, *FuncInfo, LiveRegs, MF, MBB, MBBI, DL, Reg.VGPR,
                      *Reg.FI);
   }
 
   for (auto ReservedWWM : FuncInfo->wwmAllocation()) {
     if (!ScratchExecCopy)
       ScratchExecCopy =
-          buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, /*IsProlog*/ true);
+          buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, DL, /*IsProlog*/ true);
 
     buildPrologSpill(ST, TRI, *FuncInfo, LiveRegs, MF, MBB, MBBI, DL,
                      std::get<0>(ReservedWWM), std::get<1>(ReservedWWM));
   }
 
   if (ScratchExecCopy) {
     // FIXME: Split block and make terminator.
     unsigned ExecMov = ST.isWave32() ? AMDGPU::S_MOV_B32 : AMDGPU::S_MOV_B64;
void SIFrameLowering::emitEpilogue(MachineFunction &MF,
   const SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
   if (FuncInfo->isEntryFunction())
     return;
 
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
   const SIInstrInfo *TII = ST.getInstrInfo();
   MachineRegisterInfo &MRI = MF.getRegInfo();
   const SIRegisterInfo &TRI = TII->getRegisterInfo();
-  MachineBasicBlock::iterator MBBI = MBB.getFirstTerminator();
   LivePhysRegs LiveRegs;
+  // 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.getLastNonDebugInstr();
+    if (MBBI != MBB.end())
+      DL = MBBI->getDebugLoc();
+
+    MBBI = MBB.getFirstTerminator();
+  }
 
   const MachineFrameInfo &MFI = MF.getFrameInfo();
   uint32_t NumBytes = MFI.getStackSize();
   uint32_t RoundedSize = FuncInfo->isStackRealigned()
                              ? NumBytes + MFI.getMaxAlign().value()
                              : NumBytes;
   const Register StackPtrReg = FuncInfo->getStackPtrOffsetReg();
   const Register FramePtrReg = FuncInfo->getFrameOffsetReg();
void SIFrameLowering::emitEpilogue(MachineFunction &MF,
   Register ScratchExecCopy;
   for (const SIMachineFunctionInfo::SGPRSpillVGPR &Reg :
        FuncInfo->getSGPRSpillVGPRs()) {
     if (!Reg.FI)
       continue;
 
     if (!ScratchExecCopy)
       ScratchExecCopy =
-          buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, /*IsProlog*/ false);
+          buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, DL, /*IsProlog*/ false);
 
     buildEpilogRestore(ST, TRI, *FuncInfo, LiveRegs, MF, MBB, MBBI, DL,
                        Reg.VGPR, *Reg.FI);
   }
 
   for (auto ReservedWWM : FuncInfo->wwmAllocation()) {
     if (!ScratchExecCopy)
       ScratchExecCopy =
-          buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, /*IsProlog*/ false);
+          buildScratchExecCopy(LiveRegs, MF, MBB, MBBI, DL, /*IsProlog*/ false);
 
     buildEpilogRestore(ST, TRI, *FuncInfo, LiveRegs, MF, MBB, MBBI, DL,
                        std::get<0>(ReservedWWM), std::get<1>(ReservedWWM));
   }
 
   if (ScratchExecCopy) {
     // FIXME: Split block and make terminator.
     unsigned ExecMov = ST.isWave32() ? AMDGPU::S_MOV_B32 : AMDGPU::S_MOV_B64;

llvm/test/CodeGen/AMDGPU/prologue-epilogue-markers.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -filetype=obj < %s | llvm-dwarfdump --debug-line - | FileCheck --check-prefix=DWARFLINE %s
+
+; Test that the prologue end line directive is emitted after all the prologue instructions
+; and also before the beginning of the epilogue instructions in a trivial function.
+
+; Function Attrs: convergent noinline nounwind optnone mustprogress
+define hidden void @_Z9base_casev() #0 !dbg !6 {
+; CHECK-LABEL: _Z9base_casev:
+; CHECK:       .Lfunc_begin0:
+; CHECK-NEXT:    .file 0 "dir" "file.cpp"
+; CHECK-NEXT:    .loc 0 5 0 ; file.cpp:5:0
+; CHECK-NEXT:    .cfi_sections .debug_frame
+; CHECK-NEXT:    .cfi_startproc
+; CHECK-NEXT:  ; %bb.0: ; %entry
+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    .loc 0 7 3 prologue_end ; file.cpp:7:3
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
+; CHECK-NEXT:  .Ltmp1:
+
+; DWARFLINE:    file format elf64-amdgpu
+; DWARFLINE:    .debug_line contents
+; DWARFLINE:    Address            Line   Column File   ISA Discriminator Flags
+; DWARFLINE:    0x0000000000000000      5      0      0   0             0  is_stmt
+; DWARFLINE-NEXT: 0x0000000000000004      7      3      0   0             0  is_stmt prologue_end
+; DWARFLINE-NEXT: 0x0000000000000008      7      3      0   0             0  is_stmt end_sequence
+
+entry:
+  ret void, !dbg !7
+}
+
+attributes #0 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_11, file: !1, isOptimized: false, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "file.cpp", directory: "dir")
+!2 = !DISubroutineType(types: !3)
+!3 = !{null}
+!4 = !{i32 7, !"Dwarf Version", i32 5}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "base_case", linkageName: "_Z9base_casev", scope: !1, file: !1, line: 5, type: !2, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!7 = !DILocation(line: 7, column: 3, scope: !6)