[PATCH] D41827: [DEBUG] Initial adaptation of NVPTX target for debug info emission.

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Looks OK to me. That said,  I have little clue about DWARF, so I'll defer to echristo@ as it's his domain.

In general, it would be good if we could generate debug info in a somewhat more compact way (more than one byte per .b8 directive, for example) -- debug info has tendency to grow and we do carry PTX around in compiled binaries.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2003
+      Asm->OutStreamer->EmitLabel(U.getMacroLabelBegin());
+      handleMacroNodes(CUNode->getMacros(), U);
+    }
----------------
`CUNode->getMacros()` -> `Macros`


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:263
+  /// temp symbols inside DWARF sections.
+  bool UseSectionsLabelsAsReferences = false;
+
----------------
Nit: Should it be section (singular) labels (plural) here? E.g. we have license plates and book covers. 

Perhaps it should be just `UseSectionsAsReferences`.


================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.h:350
+    bool Result = AsmPrinter::runOnMachineFunction(F);
+    OutStreamer->EmitRawText(StringRef("}\n"));
+    return Result;
----------------
Do I understand it correctly that printout of the `}` was moved here out of `EmitFunctionBodyEnd()` so that we can emit additional debug labels after the last basic block?

It would be good to add few comments describing why we emit (or not) braces in particular places.


https://reviews.llvm.org/D41827