[NFC][DebugInfo] Deprecate iterator-taking moveBefore and getFirstNonPHI (#124290)

The RemoveDIs project [0] makes debug intrinsics obsolete and to support
this instruction iterators carry an extra bit of debug information. To
maintain debug information accuracy insertion needs to be performed with a
BasicBlock::iterator rather than with Instruction pointers, otherwise the
extra bit of debug information is lost.

To that end, we're deprecating getFirstNonPHI and moveBefore for
instruction pointers. They're replaced by getFirstNonPHIIt and an
iterator-taking moveBefore: switching to the replacement is
straightforwards, and 99% of call-sites need only to unwrap the iterator
with &* or call getIterator() on an Instruction pointer.

The exception is when inserting instructions at the start of a block: if
you call getFirstNonPHI() (or begin() or getFirstInsertionPt()) and then
insert something at that position, you must pass the BasicBlock::iterator
returned into the insertion method. Unwrapping with &* and then calling
getIterator strips the debug-info bit we wish to preserve. Please do
contact us about any use case that's confusing or unclear [1].

[0] https://llvm.org/docs/RemoveDIsDebugInfo.html
[1] https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578

Author: jmorse

llvm/include/llvm/IR/BasicBlock.h

@@ -280,14 +280,27 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
   /// When adding instructions to the beginning of the basic block, they should
   /// be added before the returned value, not before the first instruction,
   /// which might be PHI. Returns 0 is there's no non-PHI instruction.
-  const Instruction* getFirstNonPHI() const;
-  Instruction* getFirstNonPHI() {
+  ///
+  /// Deprecated in favour of getFirstNonPHIIt, which returns an iterator that
+  /// preserves some debugging information.
+  LLVM_DEPRECATED("Use iterators as instruction positions", "getFirstNonPHIIt")
+  const Instruction *getFirstNonPHI() const;
+  LLVM_DEPRECATED("Use iterators as instruction positions instead",
+                  "getFirstNonPHIIt")
+  Instruction *getFirstNonPHI() {
     return const_cast<Instruction *>(
-                       static_cast<const BasicBlock *>(this)->getFirstNonPHI());
+        static_cast<const BasicBlock *>(this)->getFirstNonPHI());
   }
 
-  /// Iterator returning form of getFirstNonPHI. Installed as a placeholder for
-  /// the RemoveDIs project that will eventually remove debug intrinsics.
+  /// Returns an iterator to the first instruction in this block that is not a
+  /// PHINode instruction.
+  ///
+  /// When adding instructions to the beginning of the basic block, they should
+  /// be added before the returned value, not before the first instruction,
+  /// which might be PHI. Returns end() if there's no non-PHI instruction.
+  ///
+  /// Avoid unwrapping the iterator to an Instruction* before inserting here,
+  /// as important debug-info is preserved in the iterator.
   InstListType::const_iterator getFirstNonPHIIt() const;
   InstListType::iterator getFirstNonPHIIt() {
     BasicBlock::iterator It =

llvm/include/llvm/IR/Instruction.h

@@ -206,6 +206,12 @@ class Instruction : public User,
 
   /// Insert an unlinked instruction into a basic block immediately before
   /// the specified instruction.
+  ///
+  /// Deprecated in favour of the iterator-accepting flavour. Iterators at the
+  /// start of a block such as BasicBlock::getFirstNonPHIIt must be passed into
+  /// insertBefore without unwrapping/rewrapping. For all other positions, call
+  /// getIterator to fetch the instruction iterator.
+  LLVM_DEPRECATED("Use iterators as instruction positions", "")
   void insertBefore(Instruction *InsertPos);
 
   /// Insert an unlinked instruction into a basic block immediately before
@@ -229,6 +235,12 @@ class Instruction : public User,
 
   /// Unlink this instruction from its current basic block and insert it into
   /// the basic block that MovePos lives in, right before MovePos.
+  ///
+  /// Deprecated in favour of the iterator-accepting flavour. Iterators at the
+  /// start of a block such as BasicBlock::getFirstNonPHIIt must be passed into
+  /// moveBefore without unwrapping/rewrapping. For all other positions, call
+  /// getIterator to fetch the instruction iterator.
+  LLVM_DEPRECATED("Use iterators as instruction positions", "")
   void moveBefore(Instruction *MovePos);
 
   /// Unlink this instruction from its current basic block and insert it into
@@ -238,8 +250,20 @@ class Instruction : public User,
   /// Perform a \ref moveBefore operation, while signalling that the caller
   /// intends to preserve the original ordering of instructions. This implicitly
   /// means that any adjacent debug-info should move with this instruction.
-  /// This method is currently a no-op placeholder, but it will become
-  /// meaningful when the "RemoveDIs" project is enabled.
+  void moveBeforePreserving(InstListType::iterator MovePos);
+
+  /// Perform a \ref moveBefore operation, while signalling that the caller
+  /// intends to preserve the original ordering of instructions. This implicitly
+  /// means that any adjacent debug-info should move with this instruction.
+  void moveBeforePreserving(BasicBlock &BB, InstListType::iterator I);
+
+  /// Perform a \ref moveBefore operation, while signalling that the caller
+  /// intends to preserve the original ordering of instructions. This implicitly
+  /// means that any adjacent debug-info should move with this instruction.
+  ///
+  /// Deprecated in favour of the iterator-accepting flavour of
+  /// moveBeforePreserving, as all insertions should be at iterator positions.
+  LLVM_DEPRECATED("Use iterators as instruction positions", "")
   void moveBeforePreserving(Instruction *MovePos);
 
 private:
@@ -253,11 +277,6 @@ class Instruction : public User,
   /// \pre I is a valid iterator into BB.
   void moveBefore(BasicBlock &BB, InstListType::iterator I);
 
-  void moveBeforePreserving(BasicBlock &BB, InstListType::iterator I);
-  /// Unlink this instruction from its current basic block and insert it into
-  /// the basic block that MovePos lives in, right before MovePos.
-  void moveBeforePreserving(InstListType::iterator I);
-
   /// Unlink this instruction from its current basic block and insert it into
   /// the basic block that MovePos lives in, right after MovePos.
   void moveAfter(Instruction *MovePos);

llvm/lib/IR/BasicBlock.cpp

@@ -372,15 +372,19 @@ const Instruction* BasicBlock::getFirstNonPHI() const {
 }
 
 BasicBlock::const_iterator BasicBlock::getFirstNonPHIIt() const {
-  const Instruction *I = getFirstNonPHI();
-  if (!I)
-    return end();
-  BasicBlock::const_iterator It = I->getIterator();
-  // Set the head-inclusive bit to indicate that this iterator includes
-  // any debug-info at the start of the block. This is a no-op unless the
-  // appropriate CMake flag is set.
-  It.setHeadBit(true);
-  return It;
+  for (const Instruction &I : *this) {
+    if (isa<PHINode>(I))
+      continue;
+
+    BasicBlock::const_iterator It = I.getIterator();
+    // Set the head-inclusive bit to indicate that this iterator includes
+    // any debug-info at the start of the block. This is a no-op unless the
+    // appropriate CMake flag is set.
+    It.setHeadBit(true);
+    return It;
+  }
+
+  return end();
 }
 
 BasicBlock::const_iterator
@@ -424,11 +428,10 @@ BasicBlock::getFirstNonPHIOrDbgOrLifetime(bool SkipPseudoOp) const {
 }
 
 BasicBlock::const_iterator BasicBlock::getFirstInsertionPt() const {
-  const Instruction *FirstNonPHI = getFirstNonPHI();
-  if (!FirstNonPHI)
+  const_iterator InsertPt = getFirstNonPHIIt();
+  if (InsertPt == end())
     return end();
 
-  const_iterator InsertPt = FirstNonPHI->getIterator();
   if (InsertPt->isEHPad()) ++InsertPt;
   // Set the head-inclusive bit to indicate that this iterator includes
   // any debug-info at the start of the block. This is a no-op unless the

llvm/lib/IR/Instruction.cpp

@@ -1169,7 +1169,7 @@ bool Instruction::mayThrow(bool IncludePhaseOneUnwind) const {
     // Landingpads themselves don't unwind -- however, an invoke of a skipped
     // landingpad may continue unwinding.
     BasicBlock *UnwindDest = cast<InvokeInst>(this)->getUnwindDest();
-    Instruction *Pad = UnwindDest->getFirstNonPHI();
+    BasicBlock::iterator Pad = UnwindDest->getFirstNonPHIIt();
     if (auto *LP = dyn_cast<LandingPadInst>(Pad))
       return canUnwindPastLandingPad(LP, IncludePhaseOneUnwind);
     return false;