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

[Wei-Ren Chen via Phabricator via llvm-commits]

chenwj added a comment.

Nitpick.



================
Comment at: lib/IR/AsmWriter.cpp:1978
+  Printer.printString("name", N->getName());
+  Printer.printMetadata("file", N->getRawFile());
+  Printer.printInt("line", N->getLine());
----------------
HsiangKai wrote:
> aprantl wrote:
> > I think file and line should just come from the DILocation. Is there a situation where the File in the DILabel would not match the one in the DILocation?
> The line number in DILocation is used for Instruction’s location. The line number is DILabel is used for Label’s location. Their meanings are different. They are just happened to be the same.
> 
> I reviewed llvm.dbg.declare implementation. It seems that the line number in DILocalVariable is also redundant. I am not very clear why there existed redundant line information in DILocalVariable. The only reason I guess is they have different meanings in logic and the implementation is easier if metadata has line number in itself. Actually, the line number of variable is got from variable’s metadata instead of derived from its intrinsic’s DILocation.
> 
> There may be other reasons I did not aware. However, if it is really redundant. I think there should be another patch to review the design in all metadata instead of in this patch. I prefer to be consistent with other metadata design concept.
I agree with your last point. We can have another patch doing the cleanup, that makes things clear.


================
Comment at: lib/IR/DIBuilder.cpp:706
+
+  auto *Node =
+      DILabel::get(VMContext, cast_or_null<DILocalScope>(Context), Name,
----------------
Since you don't use `Node` after creating it, I think you can return `DILabel::get` directly.


Repository:
  rL LLVM

https://reviews.llvm.org/D45024