[PATCH] D132224: [Assignment Tracking][5/*] Add core infrastructure for instruction reference

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando marked an inline comment as done.
Orlando added a comment.

Hi @chrisjackson, thank you for taking a look at these patches! :-)



================
Comment at: llvm/include/llvm/IR/DebugInfo.h:163
+/// Assignment Tracking (at).
+namespace at {
+//
----------------
chrisjackson wrote:
> I'm bikeshedding but i think the namespace name is too terse. Also possibly not nice to read  because it's a word and an acronym
Any suggestions or thoughts on alternatives?
`astr` (or does the 'ast' or possible 'str' stand out too much?) 
`astra` (too fun?)
`asst` (not a fan)
`atra` ... etc.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:803
+    if (isa<DIAssignID>(Init)) {
+      for (auto *Inst : TempDIAssignIDAttachments[ToReplace]) {
+        assert(!Inst->getMetadata(LLVMContext::MD_DIAssignID) &&
----------------
chrisjackson wrote:
> Possibly need to assert on ToReplace?
Original loop didn't see fit to do so, so I think we're okay not to too?


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:805
+        assert(!Inst->getMetadata(LLVMContext::MD_DIAssignID) &&
+               "Inst already has DIAssignID?");
+        Inst->setMetadata(LLVMContext::MD_DIAssignID, Init);
----------------
chrisjackson wrote:
> nit, Is assert text really a question? Can you just state instead?
Sure, will do before landing if there are no major changes to make.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132224/new/

https://reviews.llvm.org/D132224