[PATCH] D45024: [DebugInfo] Add DILabel metadata and intrinsic llvm.dbg.label.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Looking pretty good so far, a couple more details inline.



================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1614
           DITemplateParameterArray TemplateParams, DISubprogram *Declaration,
-          DILocalVariableArray Variables, DITypeArray ThrownTypes,
+          DINodeArray Elements, DITypeArray ThrownTypes,
           StorageType Storage, bool ShouldCreate = true) {
----------------
Elements is a bit too nondescript. Can we call it RetainedNodes or RetainedIntrinsics (like the RetainedTypes in DICompileUnit)?


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2615
+
+  DILabel(LLVMContext &C, StorageType Storage, unsigned Line,
+          ArrayRef<Metadata *> Ops)
----------------
aprantl wrote:
> aprantl wrote:
> > Why do we need to store the Line in the DILabel? Isn't it redundant with the DILocation !dbg attachment  on the dbg.label  intrinsic?
> I don't think that it is necessary to store the line number in the DILabel. It is already stored in the !dbg DILocation attachment on the call.
This is fine now, because we retain the labels in the subprogram.


================
Comment at: include/llvm/IR/IntrinsicInst.h:178
+
+    // Methods for support type inquiry through isa, cast, and dyn_cast:
+    static bool classof(const IntrinsicInst *I) {
----------------
There's a way to express this is doxygen:
```
/// Methods for support type inquiry through isa, cast, and dyn_cast:
/// @{
...
/// @}
```


================
Comment at: lib/AsmParser/LLParser.cpp:4346
 ///                     isOptimized: false, templateParams: !4, declaration: !5,
-///                     variables: !6, thrownTypes: !7)
+///                     elements: !6, thrownTypes: !7)
 bool LLParser::ParseDISubprogram(MDNode *&Result, bool IsDistinct) {
----------------
same here: maybe `retainedNodes:` or `retainedIntrinsics:`


================
Comment at: lib/IR/DIBuilder.cpp:719
+    // to preserve label info in such situation then stash it in a
+    // named mdnode.
+    DISubprogram *Fn = getDISubprogram(Scope);
----------------
I don't understand this comment. Shouldn't it say "append it to the list of retained nodes of the DISubprogram"?


================
Comment at: lib/IR/LLVMContextImpl.h:969
+  unsigned getHashValue() const {
+    return hash_combine(Scope, Name, File, Line);
+  }
----------------
Do you think there is value in hashing the name and file an line? It seems like name+line or file+line should already be mostly unique and the comparison function will take care of collisions.


================
Comment at: test/DebugInfo/Generic/debug-label-bitcode.ll:1
+; Test bitcode writer/reader for DILabel metadata.
+; RUN: llvm-as -o - %s | llvm-dis | FileCheck %s
----------------
bitcode reader tests should go into test/Assembler and should do a double-roundtrip:

; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
; RUN: verify-uselistorder %s



Repository:
  rL LLVM

https://reviews.llvm.org/D45024