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

[jmorse]

NB: as part of this review, please review the commit message as people are going to git-blame back to it:

The RemoveDIs project has moved debugging information out of intrinsics, but metadata is sometimes required when inserting instructions at the start of blocks. To maintain accuracy, in the future instruction insertion needs to be performed with a BasicBlock::iterator, so that a debug-info bit can be stored in the iterator.

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:

https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Jeremy Morse (jmorse)

Changes

NB: as part of this review, please review the commit message as people are going to git-blame back to it:

The RemoveDIs project has moved debugging information out of intrinsics, but metadata is sometimes required when inserting instructions at the start of blocks. To maintain accuracy, in the future instruction insertion needs to be performed with a BasicBlock::iterator, so that a debug-info bit can be stored in the iterator.

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:

https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578

---

Full diff: https://github.com/llvm/llvm-project/pull/124290.diff

4 Files Affected:

• (modified) llvm/include/llvm/IR/BasicBlock.h (+14-2)
• (modified) llvm/include/llvm/IR/Instruction.h (+26-7)
• (modified) llvm/lib/IR/BasicBlock.cpp (+10-4)
• (modified) llvm/lib/IR/Instruction.cpp (+1-1)

diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index e22fe1e7e7dc8f..2d38fbabcca397 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -280,14 +280,26 @@ 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.
+  ///
+  /// 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", "getFirstNonPHIIt")
   Instruction* getFirstNonPHI() {
     return const_cast<Instruction *>(
                        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 debug-info relevant information is preserved in the iterator.
   InstListType::const_iterator getFirstNonPHIIt() const;
   InstListType::iterator getFirstNonPHIIt() {
     BasicBlock::iterator It =
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 6cdd79ce16005c..900384432d75d0 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/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);
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 0efc04cb2c8679..116869c3f6cfa9 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -372,9 +372,16 @@ const Instruction* BasicBlock::getFirstNonPHI() const {
 }
 
 BasicBlock::const_iterator BasicBlock::getFirstNonPHIIt() const {
-  const Instruction *I = getFirstNonPHI();
+  const Instruction *I = [&]() -> const Instruction * {
+    for (const Instruction &I : *this)
+      if (!isa<PHINode>(I))
+        return &I;
+    return nullptr;
+  }();
+
   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
@@ -414,11 +421,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
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 4ab47edf3ed7d2..84d9306ca67002 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/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;

[OCHyams]

Mostly LGTM, with some questions / suggestions inline

---

NB: as part of this review, please review the commit message as people are going to git-blame back to it:

I think the first para could be slightly clearer. I propose something like:

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.

[0] (Include link to the RemoveDIs transition page as well)
[1] Discourse Link

[OCHyams]

I think this function can be simplified by just folding the success-return-path into the loop (and removing the lambda). Not a big deal, YMMV.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jmorse]

Merged in 285009f as this was composing with one of the underlying commits

[jmorse]

Uh, a case of mistake identity, meant to close #124288

[jmorse]

Merged in 79499f0 with all the feedback.

[RKSimon]

Aren't we only supposed to add LLVM_DEPRECATED once all uses have been removed from trunk as it breaks Werror builds? I've always took LLVM_DEPRECATED as a signal for downstream branches?

[jmorse]

Indeed -- I've been mopping them up for a while and didn't expect anything in-tree to hit this. Will root around for buildbot failures...

[RKSimon]

It might be MSVC is being dumb about a deprecated method calling another deprecated method....

[jmorse]

I understand MSVC isn't enjoying the deprecated-function-calls-a-deprecated-function pattern, which gcc and clang don't complain about. I'll have a patch for this shortly.