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

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

Orlando added a comment.

SGTM so far, just working my way through the unittests now.



================
Comment at: llvm/include/llvm/IR/Instruction.h:74
+      const Instruction *From,
+      std::optional<simple_ilist<DPValue>::iterator> from_here = std::nullopt,
+      bool InsertAtHead = false);
----------------
nit: `from_here` -> `FromHere`


================
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);
+
----------------
Is it worth specifying in the comment the behaviour if there are multiple / none attached?


================
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);
+
----------------
Perhaps `moveBeforeImpl` as it feels more idiomatic?


================
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);
 
----------------
IMO worth adding comments describing the difference between `moveBefore` and `moveBeforePreserving`.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:803-804
+
+    The splice method is going to take all the instructions from First up to
+    Last and insert them in _front_ of Dest, forming one long list. All the
+    DPValues attached to instructions _between_ First and Last need no
----------------
up to and including or excluding Last?


================
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?
+
----------------
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 `=`?




================
Comment at: llvm/lib/IR/BasicBlock.cpp:847
+    BasicBlockDbgInfoTest.cpp.
+
+   */
----------------
Very helpful comment and diagrams!


================
Comment at: llvm/lib/IR/Instruction.cpp:84
+  // around. This would be very difficult to track for DPValues, so let's
+  // detect whether it happens. (It doesn't so far).
+  assert(!isa<DbgValueInst>(this));
----------------
I think `(It doesn't so far)` can be removed.


================
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)
----------------
I'm a bit confused about what this comment is saying, please can you explain further (here)?


================
Comment at: llvm/lib/IR/Instruction.cpp:248-249
+
+auto Instruction::getDbgValueRange() const
+    -> iterator_range<DPValue::self_iterator> {
+  BasicBlock *Parent = const_cast<BasicBlock *>(getParent());
----------------
nit: Any reason to use trailing return type here (same return type in function above doesn't)?


================
Comment at: llvm/lib/IR/Instruction.cpp:255
+  }
+  assert(Parent);
+
----------------
Hoist the assert above the `if block` since `Parent` is dereferenced there? IMO a vague string would be useful too (or comment above the assert).


================
Comment at: llvm/unittests/IR/BasicBlockDbgInfoTest.cpp:211
+  EXPECT_TRUE(BB.getFirstInsertionPt().getHeadBit());
+  BasicBlock::iterator BeginIt = BB.begin();
+  // If you launder the instruction pointer through dereferencing and then
----------------
` EXPECT_TRUE(BeginIt.getHeadBit());` here for completeness? 


================
Comment at: llvm/unittests/IR/BasicBlockDbgInfoTest.cpp:257
+
+  // Move back,
+  CInst->moveBefore(BInst);
----------------
nit: comma -> full stop? Otherwise gives the impression of an unfinished comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154353