Insert two S_NOP instructions for every high level source statement.

[kzhuravl]

Summary

Tools, such as debugger, need to pause execution based on user input (i.e. breakpoint). In order to do this, two S_NOP instructions are inserted for each high level source statement: one before first isa instruction of high level source statement, and one after last isa instruction of high level source statement. Further, debugger may replace S_NOP instructions with S_TRAP instructions based on user input.

[• tstellarAMD]

lib/Target/AMDGPU/AMDGPUInsertNopsPass.cpp

1

The file name in the header is wrong, but I think the file and the class names should be renamed to use the 'SI' prefix rather than the 'AMDGPU' prefix to be consistent with other GCN only passes.

11–19

Why do we need two passes. Can't we just insert the S_NOP instructions in the first pass?

122

Style comment. We usually use MBB as a variable name when iterating over blocks and MI when iterating over Machine Instructions. This will make it more obvious what the code is doing.

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

272

We should also be running this pass if the user specifies -g.

273

This should be added to GCNPassConfig since it is GCN only.

[• tstellarAMD]

One other question. Is there some reason why the compiler can't add the s_trap instructions instead of having the debugger add them.

[kzhuravl]

tools want to selectively substitute S_NOPs with S_TRAPs based on the user input

lib/Target/AMDGPU/AMDGPUInsertNopsPass.cpp

1

ok

11–19

this should work with different optimization levels. for o0 one pass works fine. in other opt levels instructions are reordered at different compilation stages. first pass inserts DEBUG_NOP pseudo instructions before register allocation. DEBUG_NOP pseudo instruction has isTerminator attribute, which makes reordering across DEBUG_NOPs not possible. second pass lowers DEBUG_NOPs to S_NOPs right before machine code is emitted.

122

ok

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

272

tools team specifically asked not to do this. for example, for profiling they want to run it with -g, but without inserting nops

273

ok

[• tstellarAMD]

lib/Target/AMDGPU/AMDGPUInsertNopsPass.cpp

11–19

By the time we get to running the first pass, the code will have already been re-ordered by the LLVM IR passes as well as the SelectionDAG. We also can't insert instructions with terminators in the middle of blocks, because this will break other passes (and the verifier).

Can we start with one pass and if the result isn't good enough then maybe look for other solutions?

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

272

Ok, so the nops are required for both debugging and profiling? Is the command line flag you added accessible from clang?

[kzhuravl]

After discussion with Tools, it was decided to insert two S_NOPs for each high level source statement, this way we do not have to disable any optimizations in non-O0 opt levels

[kzhuravl]

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

272

nops are only required for debugging. for profiling they want to have debug info, but no nops. command line flag is accessible from clang

[kzhuravl]

Tom's feedback

lib/Target/AMDGPU/AMDGPUInsertNopsPass.cpp

11–19

After discussion with Tools, it was decided to insert two S_NOPs for each high level source statement, this way we do not have to disable any optimizations in non-O0 opt levels. One S_NOP is inserted before first isa instruction of high level source stmt and after last isa instruction of high level source stmt. Updated the diff which includes one pass

[kzhuravl]

Minor fixes

[arsenm]

This needs some tests

lib/Target/AMDGPU/SIInsertNopsPass.cpp

23 ↗

(On Diff #48628)

I don't think this is needed

48–49 ↗

(On Diff #48628)

If you don't need to initialize dependencies, you can use INITIALIZE_PASS without _BEGIN/_END

62 ↗

(On Diff #48628)

I think this needs a better name. LineToInst?

65 ↗

(On Diff #48628)

Can you invert the condition and continue to reduce indentation

70 ↗

(On Diff #48628)

addImm should be on next line

78–81 ↗

(On Diff #48628)

I think the use of auto and second here is hard to follow. ->second should be assigned to a variable, and no auto

81 ↗

(On Diff #48628)

++MI should be new line

86 ↗

(On Diff #48628)

I would prefer having an explicit MBB variable instead of using the multiple fronds here

[kzhuravl]

Matt's feedback

[• tstellarAMD]

LGTM.

lib/Target/AMDGPU/AMDGPU.h

 void initializeAMDGPUPromoteAllocaPass(PassRegistry&);
 extern char &AMDGPUPromoteAllocaID;
 
 Pass *createAMDGPUStructurizeCFGPass();
 FunctionPass *createAMDGPUISelDag(TargetMachine &tm);
 ModulePass *createAMDGPUAlwaysInlinePass();
 ModulePass *createAMDGPUOpenCLImageTypeLoweringPass();
 FunctionPass *createAMDGPUAnnotateUniformValues();
+FunctionPass *createAMDGPUInsertDebugNopsPass();
+FunctionPass *createAMDGPULowerDebugNopsPass();
 
 void initializeSIFixControlFlowLiveIntervalsPass(PassRegistry&);
 extern char &SIFixControlFlowLiveIntervalsID;
 
 void initializeSIFixSGPRLiveRangesPass(PassRegistry&);
 extern char &SIFixSGPRLiveRangesID;
 
 void initializeAMDGPUAnnotateUniformValuesPass(PassRegistry&);
 extern char &AMDGPUAnnotateUniformValuesPassID;
 
+void initializeAMDGPUInsertDebugNopsPass(PassRegistry&);
+extern char &AMDGPUInsertDebugNopsID;
+
+void initializeAMDGPULowerDebugNopsPass(PassRegistry&);
+extern char &AMDGPULowerDebugNopsID;
+
 void initializeSIAnnotateControlFlowPass(PassRegistry&);
 extern char &SIAnnotateControlFlowPassID;
 
 void initializeSIInsertWaitsPass(PassRegistry&);
 extern char &SIInsertWaitsID;
 
 extern Target TheAMDGPUTarget;
 extern Target TheGCNTarget;

lib/Target/AMDGPU/AMDGPUInsertNopsPass.cpp

+//===-- AMDGPUAlwaysInlinePass.cpp - Promote Allocas ----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+/// \file
+/// These passes insert S_NOP instruction for each high level source statement.
+/// AMDGPUInsertDebugNops pass inserts DEBUG_NOP pseudo instructions before
+/// register allocation. AMDGPULowerDebugNops pass lowers DEBUG_NOP instructions
+/// to S_NOP instructions before machine code is emitted.
+///
+/// S_NOP for each high level source statement is needed for tools (i.e.
+/// debugger, profiler), which overwrite S_NOPs with S_TRAPs as they see fit.
+//
+//===----------------------------------------------------------------------===//
+#include "AMDGPU.h"
+#include "AMDGPUInstrInfo.h"
+#include "AMDGPUSubtarget.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/MachineInstrBuilder.h"
+using namespace llvm;
+
+//===----------------------------------------------------------------------===//
+// AMDGPU Insert Debug Nops Pass
+//===----------------------------------------------------------------------===//
+
+#define DEBUG_TYPE "amdgpu-insert-debug-nops"
+#define PASS_NAME "AMDGPU Insert Debug Nops"
+
+namespace {
+
+class AMDGPUInsertDebugNops : public MachineFunctionPass {
+public:
+  static char ID;
+
+  AMDGPUInsertDebugNops() : MachineFunctionPass(ID) { }
+  const char *getPassName() const override { return PASS_NAME; }
+
+  bool runOnMachineFunction(MachineFunction &MF) override;
+};
+
+} // anonymous namespace
+
+INITIALIZE_PASS_BEGIN(
+  AMDGPUInsertDebugNops, DEBUG_TYPE, PASS_NAME, false, false)
+INITIALIZE_PASS_END(
+  AMDGPUInsertDebugNops, DEBUG_TYPE, PASS_NAME, false, false)
+
+char AMDGPUInsertDebugNops::ID = 0;
+char &llvm::AMDGPUInsertDebugNopsID = AMDGPUInsertDebugNops::ID;
+
+FunctionPass *llvm::createAMDGPUInsertDebugNopsPass() {
+  return new AMDGPUInsertDebugNops();
+}
+
+bool AMDGPUInsertDebugNops::runOnMachineFunction(MachineFunction &MF) {
+  const AMDGPUInstrInfo *TII =
+    static_cast<const AMDGPUInstrInfo*>(MF.getSubtarget().getInstrInfo());
+
+  unsigned prevLine = 0;
+  bool modified = false;
+  for (auto &CMB : MF) {
+    for (auto &CMI : CMB) {
+      if (!CMI.isDebugValue() && CMI.getDebugLoc() &&
+            prevLine != CMI.getDebugLoc().getLine()) {
+        BuildMI(CMB, CMI, CMI.getDebugLoc(), TII->get(AMDGPU::DEBUG_NOP));
+        prevLine = CMI.getDebugLoc().getLine();
+        modified = true;
+      }
+    }
+  }
+
+  return modified;
+}
+
+#undef DEBUG_TYPE
+#undef PASS_NAME
+
+//===----------------------------------------------------------------------===//
+// AMDGPU Lower Debug Nops Pass
+//===----------------------------------------------------------------------===//
+
+#define DEBUG_TYPE "amdgpu-lower-debug-nops"
+#define PASS_NAME "AMDGPU Lower Debug Nops"
+
+namespace {
+
+class AMDGPULowerDebugNops : public MachineFunctionPass {
+public:
+  static char ID;
+
+  AMDGPULowerDebugNops() : MachineFunctionPass(ID) { }
+  const char *getPassName() const override { return PASS_NAME; }
+
+  bool runOnMachineFunction(MachineFunction &MF) override;
+};
+
+} // anonymous namespace
+
+INITIALIZE_PASS_BEGIN(
+  AMDGPULowerDebugNops, DEBUG_TYPE, PASS_NAME, false, false)
+INITIALIZE_PASS_END(
+  AMDGPULowerDebugNops, DEBUG_TYPE, PASS_NAME, false, false)
+
+char AMDGPULowerDebugNops::ID = 0;
+char &llvm::AMDGPULowerDebugNopsID = AMDGPULowerDebugNops::ID;
+
+FunctionPass *llvm::createAMDGPULowerDebugNopsPass() {
+  return new AMDGPULowerDebugNops();
+}
+
+bool AMDGPULowerDebugNops::runOnMachineFunction(MachineFunction &MF) {
+  const AMDGPUInstrInfo *TII =
+    static_cast<const AMDGPUInstrInfo*>(MF.getSubtarget().getInstrInfo());
+
+  bool modified = false;
+  for (auto &CMB : MF) {
+    auto CMI = CMB.begin();
+    while (CMI != CMB.end()) {
+      if (TII->get(CMI->getOpcode()).TSFlags & AMDGPU_FLAG_DEBUG_NOP) {
+        BuildMI(CMB, *CMI, CMI->getDebugLoc(), TII->get(AMDGPU::S_NOP))
+          .addImm(0);
+        CMI = CMB.erase(CMI);
+        modified = true;
+      } else {
+        ++CMI;
+      }
+    }
+  }
+
+  if (modified) {
+    BuildMI(MF.front(), MF.front().front(), DebugLoc(), TII->get(AMDGPU::S_NOP))
+      .addImm(0);
+  }
+
+  return modified;
+}
+
+#undef DEBUG_TYPE
+#undef PASS_NAME

lib/Target/AMDGPU/AMDGPUInstrInfo.h

namespace AMDGPU {
   LLVM_READONLY
   int16_t getNamedOperandIdx(uint16_t Opcode, uint16_t NamedIndex);
 }  // End namespace AMDGPU
 
 } // End llvm namespace
 
 #define AMDGPU_FLAG_REGISTER_LOAD  (UINT64_C(1) << 63)
 #define AMDGPU_FLAG_REGISTER_STORE (UINT64_C(1) << 62)
+#define AMDGPU_FLAG_DEBUG_NOP      (UINT64_C(1) << 61)
 
 #endif

lib/Target/AMDGPU/AMDGPUInstructions.td

 // This file contains instruction defs that are common to all hw codegen
 // targets.
 //
 //===----------------------------------------------------------------------===//
 
 class AMDGPUInst <dag outs, dag ins, string asm, list<dag> pattern> : Instruction {
   field bit isRegisterLoad = 0;
   field bit isRegisterStore = 0;
+  field bit isDebugNop = 0;
 
   let Namespace = "AMDGPU";
   let OutOperandList = outs;
   let InOperandList = ins;
   let AsmString = asm;
   let Pattern = pattern;
   let Itinerary = NullALU;
 
   let TSFlags{63} = isRegisterLoad;
   let TSFlags{62} = isRegisterStore;
+  let TSFlags{61} = isDebugNop;
 }
 
 class AMDGPUShaderInst <dag outs, dag ins, string asm, list<dag> pattern>
     : AMDGPUInst<outs, ins, asm, pattern> {
 
   field bits<32> Inst = 0xffffffff;
 
 }
def RegisterStore : AMDGPUShaderInst <
     "RegisterStore $val, $addr",
     [(AMDGPUregister_store i32:$val, addrPat:$addr, (i32 timm:$chan))]
   > {
     let isRegisterStore = 1;
   }
 }
 }
 
+def DEBUG_NOP : AMDGPUShaderInst<(outs), (ins), "DEBUG_NOP", []> {
+  let isDebugNop = 1;
+  let isTerminator = 1;
+}
+
 } // End isCodeGenOnly = 1, isPseudo = 1
 
 /* Generic helper patterns for intrinsics */
 /* -------------------------------------- */
 
 class POW_Common <AMDGPUInst log_ieee, AMDGPUInst exp_ieee, AMDGPUInst mul>
   : Pat <
   (fpow f32:$src0, f32:$src1),

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 #include "llvm/Analysis/Passes.h"
 #include "llvm/CodeGen/MachineFunctionAnalysis.h"
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/IR/LegacyPassManager.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/Scalar.h"
 #include <llvm/CodeGen/Passes.h>
 
 using namespace llvm;
 
 extern "C" void LLVMInitializeAMDGPUTarget() {
   // Register the target
   RegisterTargetMachine<R600TargetMachine> X(TheAMDGPUTarget);
   RegisterTargetMachine<GCNTargetMachine> Y(TheGCNTarget);
 
   PassRegistry *PR = PassRegistry::getPassRegistry();
   initializeSILowerI1CopiesPass(*PR);
   initializeSIFixSGPRCopiesPass(*PR);
   initializeSIFoldOperandsPass(*PR);
   initializeSIFixSGPRLiveRangesPass(*PR);
   initializeSIFixControlFlowLiveIntervalsPass(*PR);
   initializeSILoadStoreOptimizerPass(*PR);
   initializeAMDGPUAnnotateKernelFeaturesPass(*PR);
   initializeAMDGPUAnnotateUniformValuesPass(*PR);
+  initializeAMDGPUInsertDebugNopsPass(*PR);
+  initializeAMDGPULowerDebugNopsPass(*PR);
   initializeAMDGPUPromoteAllocaPass(*PR);
   initializeSIAnnotateControlFlowPass(*PR);
   initializeSIInsertWaitsPass(*PR);
   initializeSILowerControlFlowPass(*PR);
 }
 
 static std::unique_ptr<TargetLoweringObjectFile> createTLOF(const Triple &TT) {
   if (TT.getOS() == Triple::AMDHSA)
GCNTargetMachine::GCNTargetMachine(const Target &T, const Triple &TT,
                                    CodeModel::Model CM, CodeGenOpt::Level OL)
     : AMDGPUTargetMachine(T, TT, CPU, FS, Options, RM, CM, OL) {}
 
 //===----------------------------------------------------------------------===//
 // AMDGPU Pass Setup
 //===----------------------------------------------------------------------===//
 
 namespace {
+
+cl::opt<bool> InsertNops(
+  "amdgpu-insert-nops",
+  cl::desc("Insert nop instruction for each high level source statement"),
+  cl::init(false));
+
 class AMDGPUPassConfig : public TargetPassConfig {
 public:
   AMDGPUPassConfig(TargetMachine *TM, PassManagerBase &PM)
     : TargetPassConfig(TM, PM) {
 
     // Exceptions and StackMaps are not supported, so these passes will never do
     // anything.
     disablePass(&StackMapLivenessID);
createMachineScheduler(MachineSchedContext *C) const override {
     return nullptr;
   }
 
   void addIRPasses() override;
   void addCodeGenPrepare() override;
   bool addPreISel() override;
   bool addInstSelector() override;
   bool addGCPasses() override;
+  void addPreRegAlloc() override;
+  void addPreEmitPass() override;
 };
 
 class R600PassConfig : public AMDGPUPassConfig {
 public:
   R600PassConfig(TargetMachine *TM, PassManagerBase &PM)
     : AMDGPUPassConfig(TM, PM) { }
 
   bool addPreISel() override;
bool AMDGPUPassConfig::addInstSelector() {
   return false;
 }
 
 bool AMDGPUPassConfig::addGCPasses() {
   // Do nothing. GC is not supported.
   return false;
 }
 
+void AMDGPUPassConfig::addPreRegAlloc() {
+  if (InsertNops) {
+    addPass(createAMDGPUInsertDebugNopsPass());
+  }
+}
+
+void AMDGPUPassConfig::addPreEmitPass() {
+  if (InsertNops) {
+    addPass(createAMDGPULowerDebugNopsPass());
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // R600 Pass Setup
 //===----------------------------------------------------------------------===//
 
 bool R600PassConfig::addPreISel() {
   AMDGPUPassConfig::addPreISel();
   addPass(createR600TextureIntrinsicsReplacer());
   return false;
 }
 
 void R600PassConfig::addPreRegAlloc() {
+  AMDGPUPassConfig::addPreRegAlloc();
   addPass(createR600VectorRegMerger(*TM));
 }
 
 void R600PassConfig::addPreSched2() {
   const AMDGPUSubtarget &ST = *getAMDGPUTargetMachine().getSubtargetImpl();
   addPass(createR600EmitClauseMarkers(), false);
   if (ST.isIfCvtEnabled())
     addPass(&IfConverterID, false);
   addPass(createR600ClauseMergePass(*TM), false);
 }
 
 void R600PassConfig::addPreEmitPass() {
+  AMDGPUPassConfig::addPreEmitPass();
   addPass(createAMDGPUCFGStructurizerPass(), false);
   addPass(createR600ExpandSpecialInstrsPass(*TM), false);
   addPass(&FinalizeMachineBundlesID, false);
   addPass(createR600Packetizer(*TM), false);
   addPass(createR600ControlFlowFinalizer(*TM), false);
 }
 
 TargetPassConfig *R600TargetMachine::createPassConfig(PassManagerBase &PM) {
bool GCNPassConfig::addInstSelector() {
   AMDGPUPassConfig::addInstSelector();
   addPass(createSILowerI1CopiesPass());
   addPass(&SIFixSGPRCopiesID);
   addPass(createSIFoldOperandsPass());
   return false;
 }
 
 void GCNPassConfig::addPreRegAlloc() {
+  AMDGPUPassConfig::addPreRegAlloc();
+
   const AMDGPUSubtarget &ST = *getAMDGPUTargetMachine().getSubtargetImpl();
 
   // This needs to be run directly before register allocation because
   // earlier passes might recompute live intervals.
   // TODO: handle CodeGenOpt::None; fast RA ignores spill weights set by the pass
   if (getOptLevel() > CodeGenOpt::None) {
     insertPass(&MachineSchedulerID, &SIFixControlFlowLiveIntervalsID);
   }

 void GCNPassConfig::addPostRegAlloc() {
   addPass(createSIShrinkInstructionsPass(), false);
 }
 
 void GCNPassConfig::addPreSched2() {
 }
 
 void GCNPassConfig::addPreEmitPass() {
+  AMDGPUPassConfig::addPreEmitPass();
+
   addPass(createSIInsertWaitsPass(), false);
   addPass(createSILowerControlFlowPass(), false);
 }
 
 TargetPassConfig *GCNTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new GCNPassConfig(this, PM);
 }

lib/Target/AMDGPU/CMakeLists.txt

 add_llvm_target(AMDGPUCodeGen
   AMDILCFGStructurizer.cpp
   AMDGPUAlwaysInlinePass.cpp
   AMDGPUAnnotateKernelFeatures.cpp
   AMDGPUAnnotateUniformValues.cpp
   AMDGPUAsmPrinter.cpp
   AMDGPUFrameLowering.cpp
   AMDGPUTargetObjectFile.cpp
+  AMDGPUInsertNopsPass.cpp
   AMDGPUIntrinsicInfo.cpp
   AMDGPUISelDAGToDAG.cpp
   AMDGPUMCInstLower.cpp
   AMDGPUMachineFunction.cpp
   AMDGPUOpenCLImageTypeLoweringPass.cpp
   AMDGPUSubtarget.cpp
   AMDGPUTargetMachine.cpp
   AMDGPUTargetTransformInfo.cpp