[AMDGPU] Insert nop pass: take care of outstanding feedback

[kzhuravl]

Summary

• Switch few loops to range-based for loops
• Fix nop insertion at the end of BB
• Fix formatting
• Check for endpgm

[• tstellarAMD]

LGTM. Usually for post-commit feeback like this, you can just commit the changes directly instead of submitting a patch.

[arsenm]

Shouldn't the subtargets check be moved into the pass itself?

[kzhuravl]

Matt's feedback

[kzhuravl]

Fixed

llvm/trunk/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 }
 
 void GCNPassConfig::addPreSched2() {
 }
 
 void GCNPassConfig::addPreEmitPass() {
   addPass(createSIInsertWaitsPass(), false);
   addPass(createSILowerControlFlowPass(), false);
-
-  const AMDGPUSubtarget &ST = *getAMDGPUTargetMachine().getSubtargetImpl();
-  if (ST.debuggerInsertNops()) {
-    addPass(createSIInsertNopsPass(), false);
-  }
-}
 
 TargetPassConfig *GCNTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new GCNPassConfig(this, PM);
 }

llvm/trunk/lib/Target/AMDGPU/SIInsertNopsPass.cpp

 char SIInsertNops::ID = 0;
 char &llvm::SIInsertNopsID = SIInsertNops::ID;
 
 FunctionPass *llvm::createSIInsertNopsPass() {
   return new SIInsertNops();
 }
 
 bool SIInsertNops::runOnMachineFunction(MachineFunction &MF) {
+  // Skip this pass if debugger-insert-nops feature is not enabled.
+  const AMDGPUSubtarget &ST = MF.getSubtarget<AMDGPUSubtarget>();
+  if (!ST.debuggerInsertNops())
+    return false;
+
   // Skip machine functions without debug info.
-  if (!MF.getMMI().hasDebugInfo()) {
+  if (!MF.getMMI().hasDebugInfo())
     return false;
-  }
 
   // Target instruction info.
   const SIInstrInfo *TII =
     static_cast<const SIInstrInfo*>(MF.getSubtarget().getInstrInfo());
 
   // Mapping from high level source statement line number to last corresponding
   // isa instruction.
   DenseMap<unsigned, MachineBasicBlock::iterator> LineToInst;
   // Insert nop instruction before first isa instruction of each high level
   // source statement and collect last isa instruction for each high level
   // source statement.
-  for (auto MBB = MF.begin(); MBB != MF.end(); ++MBB) {
-    for (auto MI = MBB->begin(); MI != MBB->end(); ++MI) {
-      if (MI->isDebugValue() || !MI->getDebugLoc()) {
+  for (auto &MBB : MF) {
+    for (auto MI = MBB.begin(); MI != MBB.end(); ++MI) {
+      if (MI->isDebugValue() || !MI->getDebugLoc())
         continue;
-      }
+
       auto DL = MI->getDebugLoc();
       auto CL = DL.getLine();
       auto LineToInstEntry = LineToInst.find(CL);
       if (LineToInstEntry == LineToInst.end()) {
-        BuildMI(*MBB, *MI, DL, TII->get(AMDGPU::S_NOP))
+        BuildMI(MBB, *MI, DL, TII->get(AMDGPU::S_NOP))
           .addImm(0);
         LineToInst.insert(std::make_pair(CL, MI));
       } else {
         LineToInstEntry->second = MI;
       }
     }
   }
   // Insert nop instruction after last isa instruction of each high level source
   // statement.
-  for (auto LineToInstEntry = LineToInst.begin();
-         LineToInstEntry != LineToInst.end(); ++LineToInstEntry) {
-    auto MBB = LineToInstEntry->second->getParent();
-    auto DL = LineToInstEntry->second->getDebugLoc();
-    MachineBasicBlock::iterator MI = LineToInstEntry->second;
-    ++MI;
-    if (MI != MBB->end()) {
-      BuildMI(*MBB, *MI, DL, TII->get(AMDGPU::S_NOP))
+  for (auto const &LineToInstEntry : LineToInst) {
+    auto MBB = LineToInstEntry.second->getParent();
+    auto DL = LineToInstEntry.second->getDebugLoc();
+    MachineBasicBlock::iterator MI = LineToInstEntry.second;
+    if (MI->getOpcode() != AMDGPU::S_ENDPGM)
+      BuildMI(*MBB, *(++MI), DL, TII->get(AMDGPU::S_NOP))
         .addImm(0);
-    }
-  }
   // Insert nop instruction before prologue.
   MachineBasicBlock &MBB = MF.front();
   MachineInstr &MI = MBB.front();
   BuildMI(MBB, MI, DebugLoc(), TII->get(AMDGPU::S_NOP))
     .addImm(0);
 
   return true;
 }

llvm/trunk/test/CodeGen/AMDGPU/debugger_insert_nops.ll

 ; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=fiji -mattr=+amdgpu-debugger-insert-nops -verify-machineinstrs < %s | FileCheck %s
 
 ; CHECK: debugger_insert_nops.cl:2:3
 ; CHECK-NEXT: s_nop 0
-; CHECK: debugger_insert_nops.cl:3:3
+; CHECK: s_nop 0
+; CHECK-NEXT: debugger_insert_nops.cl:3:3
 ; CHECK-NEXT: s_nop 0
-; CHECK: debugger_insert_nops.cl:4:3
+; CHECK: s_nop 0
+; CHECK-NEXT: debugger_insert_nops.cl:4:3
 ; CHECK-NEXT: s_nop 0
-; CHECK: debugger_insert_nops.cl:5:3
+; CHECK: s_nop 0
+; CHECK-NEXT: debugger_insert_nops.cl:5:3
 ; CHECK-NEXT: s_nop 0
-; CHECK: debugger_insert_nops.cl:6:1
+; CHECK: s_nop 0
+; CHECK-NEXT: debugger_insert_nops.cl:6:1
 ; CHECK-NEXT: s_nop 0
 ; CHECK-NEXT: s_endpgm
 
 ; Function Attrs: nounwind
 define void @debugger_insert_nops(i32 addrspace(1)* %A) #0 !dbg !12 {
 entry:
   %A.addr = alloca i32 addrspace(1)*, align 4
   store i32 addrspace(1)* %A, i32 addrspace(1)** %A.addr, align 4