[llvm] [DebugInfo][RemoveDIs] Cope with instructions moving after themselves (PR #74113)

[Jeremy Morse via llvm-commits]

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

We occasionally move instructions to their own positions, which is not an error, and has no effect on the program. However, if dbg.value intrinsics are present then they can effectively be moved without any other effect on the program.

This is a problem if we're using non-instruction debug-info: a moveAfter that appears to be a no-op in RemoveDIs mode can jump in front of intrinsics in dbg.value mode. Thus: if an instruction is moved to itself and the head bit is set, force attached debug-info to shift down one instruction, which replicates the dbg.value behaviour.

>From f82d3962ee4144b86f4eeb1a78c25eddefcc0c3a Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Fri, 1 Dec 2023 14:21:42 +0000
Subject: [PATCH] [DebugInfo][RemoveDIs] Cope with instructions moving after
 themselves

We occasionally move instructions to their own positions, which is not an
error, and has no effect on the program. However, if dbg.value intrinsics
are present then they can effectively be moved without any other effect on
the program.

This is a problem if we're using non-instruction debug-info: a moveAfter
that appears to be a no-op in RemoveDIs mode can jump in front of
intrinsics in dbg.value mode. Thus: if an instruction is moved to itself
and the head bit is set, force attached debug-info to shift down one
instruction, which replicates the dbg.value behaviour.
---
 llvm/lib/IR/Instruction.cpp                 |  5 +-
 llvm/unittests/IR/BasicBlockDbgInfoTest.cpp | 73 +++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 4b5349856b8d7bf..7144924c34570be 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -200,8 +200,9 @@ void Instruction::moveBeforeImpl(BasicBlock &BB, InstListType::iterator I,
   // If we've been given the "Preserve" flag, then just move the DPValues with
   // the instruction, no more special handling needed.
   if (BB.IsNewDbgInfoFormat && DbgMarker && !Preserve) {
-    if (I != this->getIterator()) {
-      // "this" is definitely moving; detach any existing DPValues.
+    if (I != this->getIterator() || InsertAtHead) {
+      // "this" is definitely moving in the list, or it's moving ahead of it's
+      // attached DPValues. Detach any existing DPValues.
       handleMarkerRemoval();
     }
   }
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index a8ca706cc95b9b5..c914a5b6f239416 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -42,6 +42,79 @@ static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
 
 namespace {
 
+// We can occasionally moveAfter an instruction so that it moves to the
+// position that it already resides at. This is fine -- but gets complicated
+// with dbg.value intrinsics. By moving an instruction, we can end up changing
+// nothing but the location of debug-info intrinsics. That has to be modelled
+// by DPValues, the dbg.value replacement.
+TEST(BasicBlockDbgInfoTest, InsertAfterSelf) {
+  LLVMContext C;
+  UseNewDbgInfoFormat = true;
+
+  std::unique_ptr<Module> M = parseIR(C, R"(
+    define i16 @f(i16 %a) !dbg !6 {
+      call void @llvm.dbg.value(metadata i16 %a, metadata !9, metadata !DIExpression()), !dbg !11
+      %b = add i16 %a, 1, !dbg !11
+      call void @llvm.dbg.value(metadata i16 %b, metadata !9, metadata !DIExpression()), !dbg !11
+      %c = add i16 %b, 1, !dbg !11
+      ret i16 0, !dbg !11
+    }
+    declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+    attributes #0 = { nounwind readnone speculatable willreturn }
+
+    !llvm.dbg.cu = !{!0}
+    !llvm.module.flags = !{!5}
+
+    !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+    !1 = !DIFile(filename: "t.ll", directory: "/")
+    !2 = !{}
+    !5 = !{i32 2, !"Debug Info Version", i32 3}
+    !6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+    !7 = !DISubroutineType(types: !2)
+    !8 = !{!9}
+    !9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+    !10 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_unsigned)
+    !11 = !DILocation(line: 1, column: 1, scope: !6)
+)");
+
+  // Convert the module to "new" form debug-info.
+  M->convertToNewDbgValues();
+  // Fetch the entry block,
+  BasicBlock &BB = M->getFunction("f")->getEntryBlock();
+
+  Instruction *Inst1 = &*BB.begin();
+  Instruction *Inst2 = &*std::next(BB.begin());
+  Instruction *RetInst = &*std::next(Inst2->getIterator());
+  EXPECT_TRUE(Inst1->hasDbgValues());
+  EXPECT_TRUE(Inst2->hasDbgValues());
+  EXPECT_FALSE(RetInst->hasDbgValues());
+
+  // If we move Inst2 to be after Inst1, then it comes _immediately_ after. Were
+  // we in dbg.value form we would then have:
+  //    dbg.value
+  //    %b = add
+  //    %c = add
+  //    dbg.value
+  // Check that this is replicated by DPValues.
+  Inst2->moveAfter(Inst1);
+
+  // Inst1 should only have one DPValue on it.
+  EXPECT_TRUE(Inst1->hasDbgValues());
+  auto Range1 = Inst1->getDbgValueRange();
+  EXPECT_EQ(std::distance(Range1.begin(), Range1.end()), 1u);
+  // Inst2 should have none.
+  EXPECT_FALSE(Inst2->hasDbgValues());
+  // While the return inst should now have one on it.
+  EXPECT_TRUE(RetInst->hasDbgValues());
+  auto Range2 = RetInst->getDbgValueRange();
+  EXPECT_EQ(std::distance(Range2.begin(), Range2.end()), 1u);
+
+  M->convertFromNewDbgValues();
+
+  UseNewDbgInfoFormat = false;
+}
+
+
 TEST(BasicBlockDbgInfoTest, MarkerOperations) {
   LLVMContext C;
   UseNewDbgInfoFormat = true;