[PATCH] D20335: [AMDGPU] Emit debugger prologue and emit the rest of the debugger fields in the kernel code header

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:201-202
@@ -200,1 +200,4 @@
 
+      OutStreamer->emitRawComment(" DebuggerWavefrontPrivateSegmentOffsetSGPR: s" +
+                                  Twine(KernelInfo.DebuggerWavefrontPrivateSegmentOffsetSGPR), false);
+      OutStreamer->emitRawComment(" DebuggerPrivateSegmentBufferSGPR: s" +
----------------
This willl print the wrong thing if it isn't set. Should it print something else if not valid?

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.h:80-86
@@ -77,1 +79,9 @@
 
+    // Fixed SGPR number used to hold wave scratch offset for entire kernel
+    // execution, or uint16_t(-1) if the register is not used or not known.
+    uint16_t DebuggerWavefrontPrivateSegmentOffsetSGPR;
+    // Fixed SGPR number of the first 4 SGPRs used to hold scratch V# for entire
+    // kernel execution, or uint16_t(-1) if the register is not used or not
+    // known.
+    uint16_t DebuggerPrivateSegmentBufferSGPR;
+
----------------
kzhuravl wrote:
> Because we have to be compliant with our existing spec, which says to put SGPR number or (uint16_t)-1.
Oh, OK. I didn't realize this was in the printer

================
Comment at: lib/Target/AMDGPU/AMDGPUSubtarget.h:321
@@ +320,3 @@
+    return debuggerInsertNops() && debuggerReserveRegs()
+             && debuggerEmitPrologue();
+  }
----------------
&& on previous line

================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:42
@@ +41,3 @@
+  // specified.
+  const AMDGPUSubtarget &ST = MF.getSubtarget<AMDGPUSubtarget>();
+  if (ST.debuggerEmitPrologue())
----------------
Since my patch yesterday this should be SISubtarget, and ST can be re-used below

================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:296-297
@@ +295,4 @@
+                                           MachineBasicBlock &MBB) const {
+  const AMDGPUSubtarget &ST = MF.getSubtarget<AMDGPUSubtarget>();
+  const SIInstrInfo *TII = static_cast<const SIInstrInfo*>(ST.getInstrInfo());
+  const SIRegisterInfo *TRI = &TII->getRegisterInfo();
----------------
SISubtarget and then you can remove the casts

================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:605-606
@@ +604,4 @@
+  if (ST.debuggerEmitPrologue()) {
+    int ObjectIdx = 0;
+    // For each dimension:
+    for (unsigned i = 0; i < 3; ++i) {
----------------
Can you move the body of the loop into a helper function as well as the comment

================
Comment at: test/CodeGen/AMDGPU/debugger-emit-prologue.ll:18-19
@@ +17,4 @@
+
+; CHECK: DebuggerWavefrontPrivateSegmentOffsetSGPR: s[[SOFF]]
+; CHECK: DebuggerPrivateSegmentBufferSGPR: s[[SREG]]
+
----------------
Should also check these in a test where these aren't set


http://reviews.llvm.org/D20335