[NFC][DebugInfo] Deprecate iterator-taking moveBefore and getFirstNonPHI

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();
 }
 
 const Instruction *BasicBlock::getFirstNonPHIOrDbg(bool SkipPseudoOp) const {
@@ -414,11 +418,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;