[AMDGPU] add labels to +DumpCode output

[tpr]

Summary

+DumpCode is a hack to embed disassembly in the ELF file. This commit
fixes it to include labels, to make it slightly more useful.

[arsenm]

I still don't understand why +DumpCode is here or why it's a subtarget feature, and would like to remove it. Why do you need this? Why can't the driver just disassemble the binary itself if it wants to look at the text form

[tpr]

You're right, it's a hack, and I would like to remove it too. But first we need to get the disassembler working. The disassembler is missing some instructions, and it claims it does not support SI or CI.

In the meantime, as a short term fix, I would like to add this to make +DumpCode more useful. That will make it easier to do other work we need to do before we get to fixing the disassembler.

[tpr]

If there are no further comments on this, I will land it in the next few days.

[arsenm]

I don't think getting SI/CI to work in the disassembler will be that much work. Which instructions aren't supported? There can't be that many of them.

lib/Target/AMDGPU/AMDGPUAsmPrinter.h

200 ↗

(On Diff #123304)

Using mutable here is questionable

[tpr]

lib/Target/AMDGPU/AMDGPUAsmPrinter.h

200 ↗

(On Diff #123304)

Yes. The alternative is to do the label in EmitInstruction with a hack to tell when you have changed basic block, possibly including a non-thread-safe static variable. Would you prefer that?

In defense of using mutable, I think we all recognize +dumpcode as a hack that needs to be removed. This mutable would go with it.

[arsenm]

LGTM

llvm/trunk/lib/Target/AMDGPU/AMDGPUAsmPrinter.h

public:
 
   /// Implemented in AMDGPUMCInstLower.cpp
   void EmitInstruction(const MachineInstr *MI) override;
 
   void EmitFunctionBodyStart() override;
 
   void EmitFunctionEntryLabel() override;
 
+  void EmitBasicBlockStart(const MachineBasicBlock &MBB) const override;
+
   void EmitGlobalVariable(const GlobalVariable *GV) override;
 
   void EmitStartOfAsmFile(Module &M) override;
 
   void EmitEndOfAsmFile(Module &M) override;
 
   bool isBlockOnlyReachableByFallthrough(
     const MachineBasicBlock *MBB) const override;
 
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        unsigned AsmVariant, const char *ExtraCode,
                        raw_ostream &O) override;
 
 protected:
-  std::vector<std::string> DisasmLines, HexLines;
-  size_t DisasmLineMaxLen;
+  mutable std::vector<std::string> DisasmLines, HexLines;
+  mutable size_t DisasmLineMaxLen;
   AMDGPUAS AMDGPUASI;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_AMDGPU_AMDGPUASMPRINTER_H

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

void AMDGPUAsmPrinter::EmitFunctionEntryLabel() {
   const SIMachineFunctionInfo *MFI = MF->getInfo<SIMachineFunctionInfo>();
   const AMDGPUSubtarget &STM = MF->getSubtarget<AMDGPUSubtarget>();
   if (MFI->isEntryFunction() && STM.isAmdCodeObjectV2(*MF)) {
     SmallString<128> SymbolName;
     getNameWithPrefix(SymbolName, MF->getFunction()),
     getTargetStreamer()->EmitAMDGPUSymbolType(
         SymbolName, ELF::STT_AMDGPU_HSA_KERNEL);
   }
+  const AMDGPUSubtarget &STI = MF->getSubtarget<AMDGPUSubtarget>();
+  if (STI.dumpCode()) {
+    // Disassemble function name label to text.
+    DisasmLines.push_back(MF->getFunction()->getName().str() + ":");
+    DisasmLineMaxLen = std::max(DisasmLineMaxLen, DisasmLines.back().size());
+    HexLines.push_back("");
+  }
 
   AsmPrinter::EmitFunctionEntryLabel();
 }
 
+void AMDGPUAsmPrinter::EmitBasicBlockStart(const MachineBasicBlock &MBB) const {
+  const AMDGPUSubtarget &STI = MBB.getParent()->getSubtarget<AMDGPUSubtarget>();
+  if (STI.dumpCode() && !isBlockOnlyReachableByFallthrough(&MBB)) {
+    // Write a line for the basic block label if it is not only fallthrough.
+    DisasmLines.push_back(
+        (Twine("BB") + Twine(getFunctionNumber())
+         + "_" + Twine(MBB.getNumber()) + ":").str());
+    DisasmLineMaxLen = std::max(DisasmLineMaxLen, DisasmLines.back().size());
+    HexLines.push_back("");
+  }
+  AsmPrinter::EmitBasicBlockStart(MBB);
+}
+
 void AMDGPUAsmPrinter::EmitGlobalVariable(const GlobalVariable *GV) {
 
   // Group segment variables aren't emitted in HSA.
   if (AMDGPU::isGroupSegment(GV))
     return;
 
   AsmPrinter::EmitGlobalVariable(GV);
 }
bool AMDGPUAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   }
 
   if (STM.dumpCode()) {
 
     OutStreamer->SwitchSection(
         Context.getELFSection(".AMDGPU.disasm", ELF::SHT_NOTE, 0));
 
     for (size_t i = 0; i < DisasmLines.size(); ++i) {
-      std::string Comment(DisasmLineMaxLen - DisasmLines[i].size(), ' ');
+      std::string Comment = "\n";
+      if (!HexLines[i].empty()) {
+        Comment = std::string(DisasmLineMaxLen - DisasmLines[i].size(), ' ');
         Comment += " ; " + HexLines[i] + "\n";
+      }
 
       OutStreamer->EmitBytes(StringRef(DisasmLines[i]));
       OutStreamer->EmitBytes(StringRef(Comment));
     }
   }
 
   return false;
 }