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

[via llvm-commits]

Author: Jeremy Morse
Date: 2023-12-05T14:05:00Z
New Revision: 186695929db632b057334757923d1cab1aa395ca

URL: https://github.com/llvm/llvm-project/commit/186695929db632b057334757923d1cab1aa395ca
DIFF: https://github.com/llvm/llvm-project/commit/186695929db632b057334757923d1cab1aa395ca.diff

LOG: [DebugInfo][RemoveDIs] Cope with instructions moving after themselves (#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.

Added: 
    

Modified: 
    llvm/lib/IR/Instruction.cpp
    llvm/unittests/IR/BasicBlockDbgInfoTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index a6e43de5978b5..920a2ecfb07ee 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 its
+      // attached DPValues. Detach any existing DPValues.
       handleMarkerRemoval();
     }
   }

diff  --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index d239e3b2d83ea..46b42a257f588 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -42,6 +42,78 @@ 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;