[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.

I started reviewing this but didn't have time to finish it. For fear of losing my comments I'll post what I've got so far. Note to self: start at `Instruction.h`.



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:65
+  /// block returns to a cannonical form when the terminator is re-inserted.
+  DPMarker TrailingDPValues;
 
----------------
How frequently do we expect TrailingDPValues to be used? Is it worth having this heap allocated instead to save some bits?


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:45
+//
+//===----------------------------------------------------------------------===//
+
----------------
Comment SGTM


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:82
+///
+/// This class inherits from DebugValueUser to allow LLVMs metadata facilities
+/// to update our references to metadata beneath our feet.
----------------
nit: LLVM's


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:112-114
+  DILocalVariable *Variable;
+  DIExpression *Expression;
+  DebugLoc DbgLoc;
----------------
Looks like these fields have 'getters' already - should they be private? Or, if they don't change after ctor we could make them const instead.


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:211
+
+  void setUndef() {
+    // TODO: When/if we remove duplicate values from DIArgLists, we don't need
----------------
nit: we changed DbgVariableIntrinsic's get/setUndef to get/setKillLocation - IMO it'd be better to keep the names consistent here.


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:329
+
+  /// Implement operator<< on DPMarker.
+  void print(raw_ostream &O, bool IsForDebug = false) const;
----------------
TODO? Or can this comment be removed?


================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:342
+  /// Clone all DPMarkers from \p From into this marker. There are numerous
+  /// options to customise the source/destination, due to gnarlyness, see class
+  /// comment.
----------------
nit: Chrome's built in spell check says gnarlyness isn't a word, but it also doesn't think the result from Googling it, gnarliness, is either, so I don't know what to believe.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153990