[RemoveDIs] Prevent errors from incorrectly inserted PHIs

[SLTozer]

See discussion:
https://discourse.llvm.org/t/psa-instruction-constructors-changing-to-iterator-only-insertion/77845

PHIs should always appear at the start of a block, before anything other than other PHIs, including debug records. However, since debug records are not instructions, it is easy to insert PHIs after debug records via a common insertion pattern, insertBefore(BB->getFirstNonPHI()). Previously this triggered an assert, but in the interest of allowing a gradual change to use iterators for insertion (i.e. insertBefore(BB->getFirstNonPHIIt())), we change the insert logic here to ensure that PHIs do not appear after debug records.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Stephen Tozer (SLTozer)

Changes

See discussion:
https://discourse.llvm.org/t/psa-instruction-constructors-changing-to-iterator-only-insertion/77845

PHIs should always appear at the start of a block, before anything other than other PHIs, including debug records. However, since debug records are not instructions, it is easy to insert PHIs after debug records via a common insertion pattern, insertBefore(BB->getFirstNonPHI()). Previously this triggered an assert, but in the interest of allowing a gradual change to use iterators for insertion (i.e. insertBefore(getFirstNonPHIIt())), we change the insert logic here to ensure that PHIs do not appear after debug records.

---

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

2 Files Affected:

• (modified) llvm/lib/IR/Instruction.cpp (+9-8)
• (modified) llvm/unittests/IR/InstructionsTest.cpp (+55)

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 0602a55b9fe7f3..d2195770f0a60d 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -149,19 +149,20 @@ void Instruction::insertBefore(BasicBlock &BB,
   if (!InsertAtHead) {
     DbgMarker *SrcMarker = BB.getMarker(InsertPos);
     if (SrcMarker && !SrcMarker->empty()) {
-      // If this assertion fires, the calling code is about to insert a PHI
-      // after debug-records, which would form a sequence like:
+      // This conditional intends to fix-up cases where a PHI is inserted after
+      // debug-records, which would form a sequence like:
       //     %0 = PHI
       //     #dbg_value
       //     %1 = PHI
-      // Which is de-normalised and undesired -- hence the assertion. To avoid
-      // this, you must insert at that position using an iterator, and it must
-      // be aquired by calling getFirstNonPHIIt / begin or similar methods on
+      // Which is de-normalised and undesired -- hence the fixup here. This
+      // is a sign that the PHI is being inserted incorrectly; to insert PHIs
+      // in the right place, you must insert at an iterator, which should have
+      // been aquired by calling getFirstNonPHIIt / begin or similar methods on
       // the block. This will signal to this behind-the-scenes debug-info
-      // maintenence code that you intend the PHI to be ahead of everything,
+      // maintenance code that you intend the PHI to be ahead of everything,
       // including any debug-info.
-      assert(!isa<PHINode>(this) && "Inserting PHI after debug-records!");
-      adoptDbgRecords(&BB, InsertPos, false);
+      if (!isa<PHINode>(this))
+        adoptDbgRecords(&BB, InsertPos, false);
     }
   }
 
diff --git a/llvm/unittests/IR/InstructionsTest.cpp b/llvm/unittests/IR/InstructionsTest.cpp
index a48a51b1bf474c..27a160f4c3fbb6 100644
--- a/llvm/unittests/IR/InstructionsTest.cpp
+++ b/llvm/unittests/IR/InstructionsTest.cpp
@@ -1795,5 +1795,60 @@ TEST(InstructionsTest, InsertAtEnd) {
   EXPECT_EQ(Ret->getNextNode(), I);
 }
 
+TEST(InstructionsTest, InsertPhiAfterRecords) {
+  LLVMContext Ctx;
+  std::unique_ptr<Module> M = parseIR(Ctx, R"(
+    define i32 @f(i1 %cond) {
+    entry:
+      br i1 %cond, label %if.then, label %if.end
+
+    if.then:
+      br label %if.end
+
+    if.end:
+      %val = phi i32 [ 0, %entry ], [ 1, %if.then ]
+        #dbg_value(i32 %val, !11, !DIExpression(), !13)
+      ret i32 %val
+    }
+
+    !llvm.dbg.cu = !{!0}
+    !llvm.module.flags = !{!3, !4}
+
+    !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 19.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+    !1 = !DIFile(filename: "test.c", directory: "foo")
+    !2 = !{}
+    !3 = !{i32 2, !"Dwarf Version", i32 5}
+    !4 = !{i32 2, !"Debug Info Version", i32 3}
+    !8 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 1, type: !9, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: false, unit: !0, retainedNodes: !2)
+    !9 = !DISubroutineType(types: !10)
+    !10 = !{null}
+    !11 = !DILocalVariable(name: "val", scope: !8, file: !1, line: 2, type: !12)
+    !12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+    !13 = !DILocation(line: 2, column: 7, scope: !8)
+)");
+  M->convertToNewDbgValues();
+  Function *F = &*M->begin();
+  BasicBlock *IfEnd = &*F->begin()->getNextNode()->getNextNode();
+  Instruction *Val = &*IfEnd->begin();
+  Instruction *Ret = Val->getNextNode();
+  EXPECT_TRUE(Ret->hasDbgRecords());
+
+  // When inserting a new instruction before another instruction, the new
+  // instruction should adopt any debug records directly preceding its insert
+  // point.
+  BinaryOperator *NewMul = BinaryOperator::CreateMul(Val, Val);
+  NewMul->insertBefore(IfEnd->getFirstNonPHI());
+  EXPECT_FALSE(Ret->hasDbgRecords());
+  EXPECT_TRUE(NewMul->hasDbgRecords());
+
+  // But when inserting a PHI in the same way, we intentionally avoid adopting
+  // the debug records to prevent invalid IR; this shouldn't happen with correct
+  // iterator usage, but we want to avoid errors.
+  PHINode *NewPHI = PHINode::Create(IntegerType::get(Ctx, 32), 2);
+  NewPHI->insertBefore(IfEnd->getFirstNonPHI());
+  EXPECT_TRUE(NewMul->hasDbgRecords());
+  EXPECT_FALSE(NewPHI->hasDbgRecords());
+}
+
 } // end anonymous namespace
 } // end namespace llvm

[OCHyams]

SGTM, makes sense as a fix to create a smoother transition. Is there a plan to mark the old API functions as deprecated (I imagine there's an attribute or something?) or something similar? That's not a change for this patch, but relevant as removing this assertion probably removes some impetus to migrate to any new API.

[hvdijk]

This seems reasonable but means we no longer detect code that is currently broken, will temporarily be un-broken, but will be broken again in the future. I second @OCHyams's suggestion for marking the old insertion overloads as deprecated. I agree that should not be part of this PR, but I think we should get that in sooner rather than later.

[SLTozer]

This seems reasonable but means we no longer detect code that is currently broken, will temporarily be un-broken, but will be broken again in the future.

Yeah, this follows from the discussion linked in the patch description - the upshot is that the preferred approach would be to keep downstream code working as much as is reasonable, deprecating the old methods, and then deleting the Instruction-inserting methods and re-enabling this assert after enough time has passed (could be as short as a few weeks, or as long as a release) - I'd want the deprecation patch to land before this one, so that there's no commit range where we have neither warning.

[jryans]

@SLTozer What's the status of this? Is it still something you plan to land?

[SLTozer]

I think it should probably be closed - originally this was requested as a way to soften the transition for downstream users, by providing a safe fallback in the case that the wrong thing happens. It's been a while now and there have been no issues raised, so we're probably past the window where this would have been desirable (we've passed a successful scream test) and therefore it makes more sense to keep the assert.

[SLTozer]

Instruction-insertion has been deprecated for a full year now and this assert isn't being hit downstream, safe fallback is probably not required.