[llvm] Avoid dbg.value between phi nodes when converting from DbgRecords (PR #83620)

[Daniel Sanders via llvm-commits]

https://github.com/dsandersllvm created https://github.com/llvm/llvm-project/pull/83620

We have a downstream pass that at some point clones a phi instruction that is carrying some DPValues. When the verifier is called on IR containing phi nodes like:
```
%0 = phi ... ; has DPValues
%1 = phi ... ; cloned from previous phi
```
it currently converts to the debug intrinsics like so:
```
%0 = phi ...
call @llvm.dbg.value(...)
call @llvm.dbg.value(...)
...
%1 = phi ...
```
and then fails to verify because the phi's aren't first.

This patch moves the insertion point to a position after the phi nodes and any dbg.value calls it has already added.

It's surprising that the verifier modifies the IR but this is presumably temporary until RemoveDI's finishes. More surprising is that F.dump() performs this conversion but BB.dump() does not.

Unfortunately, I don't have a test case I can share as this problem occurred in a downstream pass and I'm not aware of any upstream that can be made to similarly clone a phi node.

>From 2ff4b2dc439489cabc707d3c307a78fe2a481fd4 Mon Sep 17 00:00:00 2001
From: Daniel Sanders <daniel_l_sanders at apple.com>
Date: Fri, 1 Mar 2024 12:46:32 -0800
Subject: [PATCH] Avoid dbg.value between phi nodes when converting from
 DbgRecords

We have a pass that at some point clones a phi instruction that is carrying
some DPValues. When the verifier is called on IR containing phi nodes like:
```
%0 = phi ... ; has DPValues
%1 = phi ... ; cloned from previous phi
```
it currently converts to the debug intrinsics like so:
```
%0 = phi ...
call @llvm.dbg.value(...)
call @llvm.dbg.value(...)
...
%1 = phi ...
```
and then fails to verify because the phi's aren't first.

This patch moves the insertion point to a position after the phi nodes and
any dbg.value calls it has already added.

It's surprising that the verifier modifies the IR but this is presumably
temporary until RemoveDI's finishes. More surprising is that F.dump()
performs this conversion but BB.dump() does not.

Unfortunately, I don't have a test case I can share as this problem occurred
in a downstream pass and I'm not aware of any upstream that can be made to
similarly clone a phi node.
---
 llvm/lib/IR/BasicBlock.cpp | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 25aa3261164513..2001804e709fc8 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -109,9 +109,14 @@ void BasicBlock::convertFromNewDbgValues() {
       continue;
 
     DPMarker &Marker = *Inst.DbgMarker;
-    for (DbgRecord &DR : Marker.getDbgValueRange())
-      InstList.insert(Inst.getIterator(),
+    for (DbgRecord &DR : Marker.getDbgValueRange()) {
+      auto I = Inst.getIterator();
+      // Avoid inserting debug info between phi nodes.
+      if (isa<PHINode>(Inst))
+        I = getFirstNonPHIOrDbg()->getIterator();
+      InstList.insert(I,
                       DR.createDebugIntrinsic(getModule(), nullptr));
+    }
 
     Marker.eraseFromParent();
   }