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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfo.h:163
+/// Assignment Tracking (at).
+namespace at {
+using AssignmentInstRange =
----------------
On the one hand, "llvm::at::" isn't the most descriptive of namespaces; but on the other hand, I don't believe llvm provides a stable C++ API, so if someone doesn't like it then it can just be changed. Good use of namespaces IMO.


================
Comment at: llvm/include/llvm/IR/DebugInfo.h:163
+/// Assignment Tracking (at).
+namespace at {
+using AssignmentInstRange =
----------------
jmorse wrote:
> On the one hand, "llvm::at::" isn't the most descriptive of namespaces; but on the other hand, I don't believe llvm provides a stable C++ API, so if someone doesn't like it then it can just be changed. Good use of namespaces IMO.
NB: a high level comment saying "Utilities for enumerating storing instructions from an assignment ID", and then from line 175 an equivalent saying "For enumerating dbg.assigns..." would make this easier to read. (IMHO, YMMV).


================
Comment at: llvm/include/llvm/IR/Instruction.h:532-533
 
+  // Update the LLVMContext ID-to-Instruction(s) mapping. If \p ID is nullptr
+  // then clear the mapping for this instruction.
+  void updateDIAssignIDMapping(DIAssignID *ID);
----------------
(Three slashes)


================
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.
----------------
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).


================
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());
----------------
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.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1652
+  return make_range(IDAsValue->user_begin(), IDAsValue->user_end());
+  ;
+}
----------------
Spurious ;


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1663-1665
+  // Replace attachments.
+  for (auto *I : getAssignmentInsts(Old))
+    I->setMetadata(LLVMContext::MD_DIAssignID, New);
----------------
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.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:86
+      return false;
+    DAI->setAddress(NewValue);
+    return true;
----------------
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?


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:94-95
   auto OldIt = find(Locations, OldValue);
-  assert(OldIt != Locations.end() && "OldValue must be a current location");
+  assert((OldIt != Locations.end() || DbgAssignAddrReplaced) &&
+         "OldValue must be a current location");
   if (!hasArgList()) {
----------------
DbgAssignAddrReplaced should be called? (It's missing parentheses)


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:179-184
+void DbgAssignIntrinsic::replaceUsesOfWith(Value *From, Value *To) {
+  if (getValue() == From)
+    setValue(To);
+  if (getAddress() == From)
+    setAddress(To);
+}
----------------
AFAIUI, and I might be very wrong here, `replaceUsesOfWith` isn't declared virtual and so only code that casts an instruction to DbgAssignIntrinsic will take this code path. Also, the base `replaceUsesOfWith` already iterates over all operands and updates them, so this might not be necessary.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:1490-1492
+  // Map DIAssignID -> Instructions with that attachment.
+  // Managed by Instruction via Instruction::updateDIAssignIDMapping.
+  // Query using the at:: functions defined in DebugInfo.h.
----------------
(Three slashes)


================
Comment at: llvm/lib/IR/Metadata.cpp:1334
+  auto &IDToInstrs = getContext().pImpl->AssignmentIDToInstrs;
+  if (auto *CurrentID =
+          cast_or_null<DIAssignID>(getMetadata(LLVMContext::MD_DIAssignID))) {
----------------
Could we make this DIAssignID * instead of auto *?

(I have this twitchy feeling because every variable in this function is auto; and all the rest of them are totally justifiable, they're container references and iterators. Except this!).


================
Comment at: llvm/lib/IR/Metadata.cpp:1342
+    auto InstrsIt = IDToInstrs.find(CurrentID);
+    assert(InstrsIt != IDToInstrs.end() && "bit out of sync with map 1");
+
----------------
Clearer assertion message please


================
Comment at: llvm/lib/IR/Metadata.cpp:1346
+    auto *InstIt = std::find(InstVec.begin(), InstVec.end(), this);
+    if (InstIt != InstVec.end()) {
+      // The vector contains a ptr to this.
----------------
IMO: can / should be an assertion, right?


================
Comment at: llvm/lib/IR/Metadata.cpp:1349
+      // If this is the only element in the vector, remove the ID:vector
+      // enrty, otherwise just remove the instruction from vector.
+      if (InstVec.size() == 1)
----------------



================
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);
----------------
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).


================
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)",
----------------
Feels better to use OpAddress etc rather than hard coded operand indexes.


================
Comment at: llvm/unittests/IR/DebugInfoTest.cpp:405
+    DbgAssignIntrinsic *First = *Markers.begin();
+    DbgAssignIntrinsic *Second = *++Markers.begin();
+    EXPECT_NE(First, Second);
----------------
std::next preferred I think (what if begin() returns a reference that gets mutated?)


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

https://reviews.llvm.org/D132224