[DebugInfo][RemoveDIs] Final cleanup for enabling non-instr-debuginfo

[jmorse]

Some final errors have turned up when doing stage2clang builds:

• We can insert before end(), which won't have an attached DPMarker, thus we can have a nullptr DPMarker in Instruction::insertBefore. Add a null check there.
• We need to use the iterator-returning form of getFirstNonPHI to ensure we don't insert any PHIs behind debug-info at the start of a block.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

Some final errors have turned up when doing stage2clang builds:

• We can insert before end(), which won't have an attached DPMarker, thus we can have a nullptr DPMarker in Instruction::insertBefore. Add a null check there.
• We need to use the iterator-returning form of getFirstNonPHI to ensure we don't insert any PHIs behind debug-info at the start of a block.

---

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

2 Files Affected:

• (modified) llvm/lib/IR/Instruction.cpp (+1-1)
• (modified) llvm/lib/Transforms/Utils/Local.cpp (+1-1)

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 920a2ecfb07ee..717e33f1857b8 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -218,7 +218,7 @@ void Instruction::moveBeforeImpl(BasicBlock &BB, InstListType::iterator I,
 
     // If we're inserting at point I, and not in front of the DPValues attached
     // there, then we should absorb the DPValues attached to I.
-    if (!InsertAtHead)
+    if (NextMarker && !InsertAtHead)
       DbgMarker->absorbDebugValues(*NextMarker, false);
   }
 
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index dfc78aa589ef8..51f39e0ba0cce 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1295,7 +1295,7 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
     // the same predecessors BB had.
     // Copy over any phi, debug or lifetime instruction.
     BB->getTerminator()->eraseFromParent();
-    Succ->splice(Succ->getFirstNonPHI()->getIterator(), BB);
+    Succ->splice(Succ->getFirstNonPHIIt(), BB);
   } else {
     while (PHINode *PN = dyn_cast<PHINode>(&BB->front())) {
       // We explicitly check for such uses for merging phis.

[OCHyams]

Should we add/amend a unittest for the absorption fix?

[OCHyams]

Perhaps this check NextMarker != nullptr should move outside this if, to prevent creating a marker on this if there's nothing to absorb? YMMV, not essential.

[OCHyams]

Classic

[OCHyams]

Ideally we could also check there's no marker added. But that may not be possible / reasonable if marker usage is not optimized yet.

[jmorse]

Both outstanding comments -> answer is that we're not shooting for marker-efficiency yet, which then means it's necessary for markers to exist on all instructions in DDD/RemoveDIs mode.