[PATCH] D154353: [DebugInfo][RemoveDIs] Implement behind-the-scenes debug-info maintenance in splice / moveBefore / insertBefore APIs

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/include/llvm/IR/Instruction.h:86-87
+
+  /// Erase a single DPValue \p I that is attached to this instruction.
+  void dropOneDbgValue(DPValue *I);
+
----------------
Orlando wrote:
> Is it worth specifying in the comment the behaviour if there are multiple / none attached?
IMO no, as the DPValue argument should fairly obviously be the DPValue that's to be dropped -- it's not "drop one random" or "drop the first DPValue" after all.


================
Comment at: llvm/include/llvm/IR/Instruction.h:190
+  /// centralising debug-info updates into one place.
+  void moveBefore1(BasicBlock &BB, InstListType::iterator I, bool Preserve);
+
----------------
Orlando wrote:
> Perhaps `moveBeforeImpl` as it feels more idiomatic?
SGTM


================
Comment at: llvm/include/llvm/IR/Instruction.h:197
   void moveBefore(BasicBlock &BB, InstListType::iterator I);
-  void moveBeforePreserving(BasicBlock &BB, InstListType::iterator I) {
-    moveBefore(BB, I);
-  }
+  void moveBeforePreserving(BasicBlock &BB, InstListType::iterator I);
 
----------------
Orlando wrote:
> IMO worth adding comments describing the difference between `moveBefore` and `moveBeforePreserving`.
(Inserting a comment about the earlier moveBeforePreserving, and referring to it from here.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:809-810
+    DPValues be transferred, and if so to where? Do we move the ":" DPValues?
+    Would they go in front of the "=" DPValues, or should those go before "+"
+    DPValues?
+
----------------
Orlando wrote:
> I'm not sure I understand this final option - why would we need to move `:` them up ahead so far? Ah, is "those" in
> 
> > or should those go before "+"  DPValues?
> 
> referring to `=`?
> 
> 
Good spot, "those" is ==== here, I'll make that explicit.


================
Comment at: llvm/lib/IR/Instruction.cpp:118-119
+
+  // No need to manually update DPValues: if we insert after an instruction
+  // position, then we can never have any DPValues on "this".
+  if (DestParent->IsNewDbgInfoFormat)
----------------
Orlando wrote:
> I'm a bit confused about what this comment is saying, please can you explain further (here)?
My recollection is that there are no scenarios where DPValues can change what instruction they're attached to as a result of insertAfter, for example:

    call dbg.value
    %foo = add
    call dbg.value
    %bar = sub
    call dbg.value

Inserting after %foo or after %bar doesn't cause an instruction be inserted inbetween dbg.values and the next instruction. All the dbg.values continue to "happen" at the next instruction, and that's preserved using DPValues too.

Of course, if the insert position of an instruction is a dbg.value intrinsic then this is no longer true. Not allowing people to do that is one of the design goals though: doing that is always a bug.


================
Comment at: llvm/lib/IR/Instruction.cpp:248-249
+
+auto Instruction::getDbgValueRange() const
+    -> iterator_range<DPValue::self_iterator> {
+  BasicBlock *Parent = const_cast<BasicBlock *>(getParent());
----------------
Orlando wrote:
> nit: Any reason to use trailing return type here (same return type in function above doesn't)?
I think there was a reason once, but it's since shifted X_X


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

https://reviews.llvm.org/D154353