[DebugInfo][RemoveDIs] Reverse order of DPValues from findDbgUsers

[jmorse]

The order of dbg.value intrinsics appearing in the output can affect the order of tables in DWARF sections. This means that DPValues, our dbg.value replacement, needs to obey the same ordering rules. For dbg.values returned by findDbgUsers it's reverse order of creation (due to how they're put on use-lists). Produce that order from findDbgUsers for DPValues.

I've got a few IR files where the order of dbg.values flips, but it's a fragile test -- ultimately it needs the number of times a DPValue is handled by findDbgValues to be odd.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

The order of dbg.value intrinsics appearing in the output can affect the order of tables in DWARF sections. This means that DPValues, our dbg.value replacement, needs to obey the same ordering rules. For dbg.values returned by findDbgUsers it's reverse order of creation (due to how they're put on use-lists). Produce that order from findDbgUsers for DPValues.

I've got a few IR files where the order of dbg.values flips, but it's a fragile test -- ultimately it needs the number of times a DPValue is handled by findDbgValues to be odd.

---

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

3 Files Affected:

• (modified) llvm/lib/IR/Metadata.cpp (+6-1)
• (modified) llvm/lib/Transforms/Utils/Local.cpp (+1-8)
• (modified) llvm/unittests/IR/DebugInfoTest.cpp (+65)

diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 415e256c817b82f..7bc25e30b893271 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -249,8 +249,13 @@ SmallVector<DPValue *> ReplaceableMetadataImpl::getAllDPValueUsers() {
       continue;
     DPVUsersWithID.push_back(&UseMap[Pair.first]);
   }
+  // Order DPValue users in reverse-creation order. Normal dbg.value users
+  // of MetadataAsValues are ordered by their UseList, i.e. reverse order of
+  // when they were added: we need to replicate that here. The structure of
+  // debug-info output depends on the ordering of intrinsics, thus we need
+  // to keep them consistent for comparisons sake.
   llvm::sort(DPVUsersWithID, [](auto UserA, auto UserB) {
-    return UserA->second < UserB->second;
+    return UserA->second > UserB->second;
   });
   SmallVector<DPValue *> DPVUsers;
   for (auto UserWithID : DPVUsersWithID)
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index e399329a58873e7..dfc78aa589ef828 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -2587,14 +2587,7 @@ static bool rewriteDebugUsers(
     }
 
     // DPValue implementation of the above.
-    // RemoveDIs misery: The above loop of intrinsic-users are ordered by the
-    // use-list of the corresponding metadata-as-value: in reverse order of when
-    // they were added. Wheras DPUsers are ordered by when they were added to
-    // the replaceable-metadata map, i.e., in the order they were added. Thus to
-    // have matching orders between the two, we have to reverse here. For
-    // RemoveDIs we might in the long run need to consider whether this implicit
-    // ordering is relied upon by any other part of LLVM.
-    for (auto *DPV : llvm::reverse(DPUsers)) {
+    for (auto *DPV : DPUsers) {
       Instruction *MarkedInstr = DPV->getMarker()->MarkedInstr;
       Instruction *NextNonDebug = MarkedInstr;
       // The next instruction might still be a dbg.declare, skip over it.
diff --git a/llvm/unittests/IR/DebugInfoTest.cpp b/llvm/unittests/IR/DebugInfoTest.cpp
index a408c0af5623b3a..be8f590a27eb4df 100644
--- a/llvm/unittests/IR/DebugInfoTest.cpp
+++ b/llvm/unittests/IR/DebugInfoTest.cpp
@@ -284,6 +284,71 @@ TEST(MetadataTest, DeleteInstUsedByDPValue) {
   UseNewDbgInfoFormat = OldDbgValueMode;
 }
 
+// Ensure that the order of dbg.value intrinsics returned by findDbgValues, and
+// their corresponding DPValue representation, are consistent.
+TEST(MetadataTest, OrderingOfDPValues) {
+  LLVMContext C;
+  std::unique_ptr<Module> M = parseIR(C, R"(
+    define i16 @f(i16 %a) !dbg !6 {
+      %b = add i16 %a, 1, !dbg !11
+      call void @llvm.dbg.value(metadata i16 %b, metadata !9, metadata !DIExpression()), !dbg !11
+      call void @llvm.dbg.value(metadata i16 %b, metadata !12, metadata !DIExpression()), !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: "foo", 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)
+    !12 = !DILocalVariable(name: "bar", scope: !6, file: !1, line: 1, type: !10)
+)");
+
+  bool OldDbgValueMode = UseNewDbgInfoFormat;
+  UseNewDbgInfoFormat = true;
+  Instruction &I = *M->getFunction("f")->getEntryBlock().getFirstNonPHI();
+
+  SmallVector<DbgValueInst *, 2> DVIs;
+  SmallVector<DPValue *, 2> DPVs;
+  findDbgValues(DVIs, &I, &DPVs);
+  ASSERT_EQ(DVIs.size(), 2u);
+  ASSERT_EQ(DPVs.size(), 0u);
+
+  // The correct order of dbg.values is given by their use-list, which becomes
+  // the reverse order of creation. Thus the dbg.values should come out as
+  // "bar" and then "foo".
+  DILocalVariable *Var0 = DVIs[0]->getVariable();
+  EXPECT_TRUE(Var0->getName() == "bar");
+  DILocalVariable *Var1 = DVIs[1]->getVariable();
+  EXPECT_TRUE(Var1->getName() == "foo");
+
+  // Now try again, but in DPValue form.
+  DVIs.clear();
+
+  M->convertToNewDbgValues();
+  findDbgValues(DVIs, &I, &DPVs);
+  ASSERT_EQ(DVIs.size(), 0u);
+  ASSERT_EQ(DPVs.size(), 2u);
+
+  Var0 = DPVs[0]->getVariable();
+  EXPECT_TRUE(Var0->getName() == "bar");
+  Var1 = DPVs[1]->getVariable();
+  EXPECT_TRUE(Var1->getName() == "foo");
+
+  M->convertFromNewDbgValues();
+  UseNewDbgInfoFormat = OldDbgValueMode;
+}
+
 TEST(DIBuiler, CreateFile) {
   LLVMContext Ctx;
   std::unique_ptr<Module> M(new Module("MyModule", Ctx));

[OCHyams]

LGTM