[PATCH] D153990: [DebugInfo][RemoveDIs] Add prototype storage classes for non-instruction variable debug-info

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

Orlando added a comment.

Code changes LGTM pending inline comments. I haven't looked at the tests yet.



================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:118
+
+  /// Create a new DPValue representing the assignment in dbg.value \p DVI.
+  DPValue(const DbgVariableIntrinsic *DVI);
----------------
Looking at the constructor definition (and the parameter type), this comment is possibly out of date? DVI is not necessarily a `dbg.value`.


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:121-122
+  DPValue(const DPValue &DPV);
+  /// Directly construct a new DPValue representing the specified assignment.
+  DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
+          const DILocation *DI);
----------------
Might be worth mentioning something about assuming this is a `dbg.value`?


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:344
+  /// comment.
+  /// \p from_here If non-null, copy from from_here to the end of Froms DPValues
+  /// \p InsertAtHead Place the cloned DPValues at the start of StoredDPValues
----------------
`from_here` -> `FromHere`?
`Froms` -> `From's`



================
Comment at: llvm/include/llvm/IR/Instruction.h:56-58
+  /// Optional marker recording the position for debugging information that
+  /// "happens" immediately before this instruction. Null unless there is
+  /// debugging information present.
----------------
Not sure about "happens" but can't come up with anything better myself. For debugging information that takes effect before this instruction? For variable locations that start before this instruction? I'll leave it with you, feel free to ignore this.


================
Comment at: llvm/include/llvm/IR/Metadata.h:220
+  void handleChangedValue(Metadata *NewDebugValue);
+  ///////////
+  DebugValueUser() = default;
----------------
`///////////`


================
Comment at: llvm/include/llvm/IR/Metadata.h:272
+
+  ///////////
+private:
----------------
`///////////`


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:2100-2102
+    MDNode *UniqueArgList = uniquify();
+    if (UniqueArgList != this) {
+      replaceAllUsesWith(UniqueArgList);
----------------
Please could you explain this change to me (why/what it's doing)? It seems the couple of months I've not touched metadata have clouded my memory.


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:22
+    break;
+  }
+  case Intrinsic::dbg_declare: {
----------------
Can we drop these braces? IMO they are just a bit of added clutter here but YMMV, feel free to ignore.


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:40-42
+    : DebugValueUser(Location), Variable(DV), Expression(Expr), DbgLoc(DI) {
+  Type = LocationType::Value;
+}
----------------
Any reason Type isn't initialized in the member initializer list like the other fields? And is it right to assume `LocationType::Value`?


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:63
+
+  // Operand must be an empty metadata tuple, so return empty iterator.
+  return {location_op_iterator(static_cast<ValueAsMetadata *>(nullptr)),
----------------
IMO worth asserting this, since metadata tends to be tricksy. wdyt?


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:180
+    break;
+  }
+  case DPValue::LocationType::Value: {
----------------
(personal preference nit: unnecessary braces, ymmv, feel free to ignore) 


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:270
+  eraseFromParent();
+  Owner->DbgMarker = nullptr;
+  return;
----------------
I think this line is redundant (`eraseFromParent` -> `removeFromParent` -> `MarkedInstr->DbgMarker = nullptr`?).


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:271
+  Owner->DbgMarker = nullptr;
+  return;
+}
----------------
nit: Unnecessary return


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:328
+  if (InsertAtHead) {
+    for (DPValue &DPV : reverse(Range)) {
+      DPValue *New = DPV.clone();
----------------
Why reverse here? (And if it's not needed, can the `for`s be merged with a conditional insert pos?)


================
Comment at: llvm/lib/IR/Metadata.cpp:410
 ReplaceableMetadataImpl *ReplaceableMetadataImpl::getOrCreate(Metadata &MD) {
+  if (auto ArgList = dyn_cast<DIArgList>(&MD)) {
+    return ArgList->Context.getOrCreateReplaceableUses();
----------------
nit: unnecessary braces, same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153990