[llvm] [DebugInfo][RemoveDIs] Assert if we mix PHIs and debug-info (PR #84054)

[Jeremy Morse via llvm-commits]

https://github.com/jmorse created https://github.com/llvm/llvm-project/pull/84054

A potentially erroneous code construction with the work we've done to remove debug intrinsics, is inserting PHIs into blocks when the position hasn't been "sourced correctly". Specifically, if you have:

    %foo = PHI
    #dbg_value
    %bar = add i32...

And plan on inserting a new PHI, you have to use the iterator form of `getFirstNonPHI` or getFirstInsertionPt (or begin()) to acquire an iterator that tells the debug-info maintenance code "this is supposed to be at the start of the block, put it in front of #dbg_value". We can detect call-sites that aren't doing this at runtime, and should do with this assertion. It might invalidate code that's doing something very unexpected, like walking backwards to find a PHI, then going forwards, then inserting: however that's just an inefficient way of calling `getFirstNonPHI`.

~

While we can detect this at runtime, it's sort of a sticking plaster. It seems like we might have to end up converting `getFirstNonPHI` to return an iterator, which is going to be annoying (128 call-sites in llvm/lib/Transforms). However this is all the natural consequence of putting debug-info correctness into the type system which is why we're doing this anyway.

>From ed8c3af96e557ea850bd21d763bd8ce2cd829e12 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Tue, 5 Mar 2024 18:11:02 +0000
Subject: [PATCH] [DebugInfo][RemoveDIs] Assert if we mix PHIs and debug-info

A potentially erronous code construction with the work we've done to remove
debug intrinsics, is inserting PHIs into blocks when the position hasn't
been "sourced correctly". Specifically, if you have:

    %foo = PHI
    #dbg_value
    %bar = add i32...

And plan on inserting a new PHI, you have to use the iterator form of
getFirstNonPHI or getFirstInsertionPt (or begin()) to acquire an iterator
that tells the debug-info maintenence code "this is supposed to be at the
start of the block, put it in front of #dbg_value". We can detect
call-sites that aren't doing this at runtime, and should do with this
assertion. It might invalidate code that's doing something very unexpected,
like walking backwards to find a PHI, then going forwards, then inserting:
however that's just an inefficient way of calling getFirstNonPHI.
---
 llvm/lib/IR/Instruction.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index ce221758ef798b..e863ef3eb8d6d7 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -149,6 +149,18 @@ void Instruction::insertBefore(BasicBlock &BB,
   if (!InsertAtHead) {
     DPMarker *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:
+      //     %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
+      // the block. This will signal to this behind-the-scenes debug-info
+      // maintenence code that you intend the PHI to be ahead of everything,
+      // including any debug-info.
+      assert(!isa<PHINode>(this) && "Inserting PHI after debug-records!");
       adoptDbgValues(&BB, InsertPos, false);
     }
   }