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

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

Orlando added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:288-291
+  // Get the FragmentInfo for the variable. Note that dbg.assign intrinsics
+  // track two DIExpressions, but only the first expression has FragmentInfo.
+  // Using this function rather than going through DIExpression directly helps
+  // to abstract/hide that hack. FIXME: Move fragment info out of DIExpression.
----------------
jmorse wrote:
> IMO can be condensed to just "This method abstracts where the fragment is stored, for intrinsics with more than one expression" or something that doesn't name specific instructions. (The concern being that the comment will rot rapidly if it's too specific).
That change leaked into this patch from the previous patch in the series (D132223) that added the dbg.assign boilerplate, sorry for the noise.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1646-1649
+  // The ID is only used wrapped in MetadataAsValue(ID), so lets check that
+  // one of those already exists first.
+  if (!IDAsValue)
+    return make_range(Value::user_iterator(), Value::user_iterator());
----------------
jmorse wrote:
> Depending on how this API is to be used, would it be better to assert that an existing DIAssignID is always used? "Fail fast fail hard" works pretty well to detect deep errors.
This isn't a error state - you can get into the situation where you have a `DIAssignID` attachment on a function which is not used by any `llvm.dbg.assign` intrinsics. For instance, if a `llvm.dbg.assign` has been deleted due to living in a dead block -- whether or not _that_ is desirable needs to be looked at on a case-by-case basis IMO, so an assert would be too broad.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1663-1665
+  // Replace attachments.
+  for (auto *I : getAssignmentInsts(Old))
+    I->setMetadata(LLVMContext::MD_DIAssignID, New);
----------------
jmorse wrote:
> Is there a risk that the SmallVector providing storage to the range being iterated on, is re-allocated / invalidates-iterators during the call to setMetadata, seeing how setMetadata -> updateDIAssignIDMapping -> erases / clears parts of the SmallVector?
> 
> If so, might be best replaced with something that modifies AssignmentIDToInstrs directly.
Good point, and I've added some "Iterators invalidated by ..." comments to the `getAssignment...` functions.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:86
+      return false;
+    DAI->setAddress(NewValue);
+    return true;
----------------
jmorse wrote:
> This has a side-effect, but is called in an assertion -- that'll mean the side-effect only happens when LLVM is built with assertions, which is presumably undesirable?
This stuff is from the previous patch in the stack, sorry!


================
Comment at: llvm/lib/IR/Verifier.cpp:4438-4441
+  // All of the dbg.assign intrinsics should be in the same function as I.
+  for (DbgAssignIntrinsic *DAI : at::getAssignmentMarkers(&I))
+    AssertDI(DAI->getFunction() == I.getFunction(),
+             "dbg.assign not in same function as inst", DAI, &I);
----------------
jmorse wrote:
> Possibly stupid question, but isn't the set iterated over here the same as in the users list above? Is it worth putting the AssertDI inside the loop above? (Feels like a massive nit pick over a tiny performance thing, feel free to ignore).
Not a stupid question, they are the same. I don't mind either way so I've changed it to your preference. I've also updated the verifier tests added in recent updates to earlier patches to check this codepath.


================
Comment at: llvm/lib/IR/Verifier.cpp:5667-5674
+             DAI->getAssignId());
+    auto *Addr = cast<MetadataAsValue>(DII.getArgOperand(4))->getMetadata();
+    AssertDI(isa<ValueAsMetadata>(Addr),
+             "invalid llvm.dbg.assign intrinsic address (5th arg)", &DII, Addr);
+    auto *AddrExpr = cast<MetadataAsValue>(DII.getArgOperand(5))->getMetadata();
+    AssertDI(isa<DIExpression>(AddrExpr),
+             "invalid llvm.dbg.assign intrinsic address expression (6th arg)",
----------------
jmorse wrote:
> Feels better to use OpAddress etc rather than hard coded operand indexes.
This also comes from the previous patch in the stack, but I will apply this suggestion to it over there.


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

https://reviews.llvm.org/D132224