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 ↗

(On Diff #48502)

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 ↗

(On Diff #48502)

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

122 ↗

(On Diff #48502)

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

269

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

270

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 ↗

(On Diff #48502)

ok

11–19 ↗

(On Diff #48502)

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 ↗

(On Diff #48502)

ok

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

269

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

270

ok

[• tstellarAMD]

lib/Target/AMDGPU/AMDGPUInsertNopsPass.cpp

11–19 ↗

(On Diff #48502)

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

269

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

269

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 ↗

(On Diff #48502)

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

24

I don't think this is needed

49–50

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

63

I think this needs a better name. LineToInst?

66

Can you invert the condition and continue to reduce indentation

71

addImm should be on next line

79–82

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

82

++MI should be new line

87

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

 FunctionPass *createSILowerI1CopiesPass();
 FunctionPass *createSIShrinkInstructionsPass();
 FunctionPass *createSILoadStoreOptimizerPass(TargetMachine &tm);
 FunctionPass *createSILowerControlFlowPass();
 FunctionPass *createSIFixControlFlowLiveIntervalsPass();
 FunctionPass *createSIFixSGPRCopiesPass();
 FunctionPass *createSIFixSGPRLiveRangesPass();
 FunctionPass *createSICodeEmitterPass(formatted_raw_ostream &OS);
+FunctionPass *createSIInsertNopsPass();
 FunctionPass *createSIInsertWaitsPass();
 
 ScheduleDAGInstrs *createSIMachineScheduler(MachineSchedContext *C);
 
 ModulePass *createAMDGPUAnnotateKernelFeaturesPass();
 void initializeAMDGPUAnnotateKernelFeaturesPass(PassRegistry &);
 extern char &AMDGPUAnnotateKernelFeaturesID;
 

 extern char &SIFixSGPRLiveRangesID;
 
 void initializeAMDGPUAnnotateUniformValuesPass(PassRegistry&);
 extern char &AMDGPUAnnotateUniformValuesPassID;
 
 void initializeSIAnnotateControlFlowPass(PassRegistry&);
 extern char &SIAnnotateControlFlowPassID;
 
+void initializeSIInsertNopsPass(PassRegistry&);
+extern char &SIInsertNopsID;
+
 void initializeSIInsertWaitsPass(PassRegistry&);
 extern char &SIInsertWaitsID;
 
 extern Target TheAMDGPUTarget;
 extern Target TheGCNTarget;
 
 namespace AMDGPU {
 enum TargetIndex {

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);
   initializeAMDGPUPromoteAllocaPass(*PR);
   initializeSIAnnotateControlFlowPass(*PR);
+  initializeSIInsertNopsPass(*PR);
   initializeSIInsertWaitsPass(*PR);
   initializeSILowerControlFlowPass(*PR);
 }
 
 static std::unique_ptr<TargetLoweringObjectFile> createTLOF(const Triple &TT) {
   if (TT.getOS() == Triple::AMDHSA)
     return make_unique<AMDGPUHSATargetObjectFile>();
 
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 two nop instructions 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);

 }
 
 bool AMDGPUPassConfig::addGCPasses() {
   // Do nothing. GC is not supported.
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // R600 Pass Setup
 //===----------------------------------------------------------------------===//
 
 bool R600PassConfig::addPreISel() {
   AMDGPUPassConfig::addPreISel();
   addPass(createR600TextureIntrinsicsReplacer());
   return false;
 }
 
 void R600PassConfig::addPreRegAlloc() {

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

lib/Target/AMDGPU/CMakeLists.txt

add_llvm_target(AMDGPUCodeGen
   R600RegisterInfo.cpp
   R600TextureIntrinsicsReplacer.cpp
   SIAnnotateControlFlow.cpp
   SIFixControlFlowLiveIntervals.cpp
   SIFixSGPRCopies.cpp
   SIFixSGPRLiveRanges.cpp
   SIFoldOperands.cpp
   SIFrameLowering.cpp
+  SIInsertNopsPass.cpp
   SIInsertWaits.cpp
   SIInstrInfo.cpp
   SIISelLowering.cpp
   SILoadStoreOptimizer.cpp
   SILowerControlFlow.cpp
   SILowerI1Copies.cpp
   SIMachineFunctionInfo.cpp
   SIMachineScheduler.cpp

lib/Target/AMDGPU/SIInsertNopsPass.cpp

+//===--- SIInsertNopsPass.cpp - Use predicates for control flow -----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+/// \file
+/// \brief Insert two S_NOP instructions for every high level source statement.
+///
+/// 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.
+//
+//===----------------------------------------------------------------------===//
+
+#include "SIInstrInfo.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/MachineInstrBuilder.h"
+using namespace llvm;
+
+#define DEBUG_TYPE "si-insert-nops"
+#define PASS_NAME "SI Insert Nops"
+
+namespace {
+
+class SIInsertNops : public MachineFunctionPass {
+public:
+  static char ID;
+
+  SIInsertNops() : MachineFunctionPass(ID) { }
+  const char *getPassName() const override { return PASS_NAME; }
+
+  bool runOnMachineFunction(MachineFunction &MF) override;
+};
+
+} // anonymous namespace
+
+INITIALIZE_PASS(SIInsertNops, DEBUG_TYPE, PASS_NAME, false, false)
+
+char SIInsertNops::ID = 0;
+char &llvm::SIInsertNopsID = SIInsertNops::ID;
+
+FunctionPass *llvm::createSIInsertNopsPass() {
+  return new SIInsertNops();
+}
+
+bool SIInsertNops::runOnMachineFunction(MachineFunction &MF) {
+  const SIInstrInfo *TII =
+    static_cast<const SIInstrInfo*>(MF.getSubtarget().getInstrInfo());
+
+  DenseMap<unsigned, MachineBasicBlock::iterator> LineToInst;
+  for (auto MBB = MF.begin(); MBB != MF.end(); ++MBB) {
+    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))
+          .addImm(0);
+        LineToInst.insert(std::make_pair(CL, MI));
+      } else {
+        LineToInstEntry->second = MI;
+      }
+    }
+  }
+  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))
+        .addImm(0);
+    }
+  }
+  MachineBasicBlock &MBB = MF.front();
+  MachineInstr &MI = MBB.front();
+  BuildMI(MBB, MI, DebugLoc(), TII->get(AMDGPU::S_NOP))
+    .addImm(0);
+
+  return true;
+}