[DebugInfo][RemoveDIs] Implement redundant elimination for DPValues

[jmorse]

This pass steps through a block forwards and backwards, identifying those variable assignment records that are redundant, and erases them, saving us a decent wedge of compile-time. This patch re-implements it to use the replacement for DbgValueInsts, DPValues, in an almost identical way.

Alas the test I've added the try-remove-dis flag to is the only one I've been able to find that manually runs this pass.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

This pass steps through a block forwards and backwards, identifying those variable assignment records that are redundant, and erases them, saving us a decent wedge of compile-time. This patch re-implements it to use the replacement for DbgValueInsts, DPValues, in an almost identical way.

Alas the test I've added the try-remove-dis flag to is the only one I've been able to find that manually runs this pass.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/BasicBlockUtils.cpp (+63)
• (modified) llvm/test/Transforms/DCE/dbg-value-removal.ll (+1)

diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 05ff4efb7b94471..b64029f2560db5f 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -382,7 +382,38 @@ bool llvm::MergeBlockSuccessorsIntoGivenBlocks(
 /// - Check fully overlapping fragments and not only identical fragments.
 /// - Support dbg.declare. dbg.label, and possibly other meta instructions being
 ///   part of the sequence of consecutive instructions.
+static bool DPValuesRemoveRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
+  SmallVector<DPValue *, 8> ToBeRemoved;
+  SmallDenseSet<DebugVariable> VariableSet;
+  for (auto &I : reverse(*BB)) {
+    for (DPValue &DPV : reverse(I.getDbgValueRange())) {
+      DebugVariable Key(DPV.getVariable(),
+                        DPV.getExpression(),
+                        DPV.getDebugLoc()->getInlinedAt());
+      auto R = VariableSet.insert(Key);
+      // If the same variable fragment is described more than once it is enough
+      // to keep the last one (i.e. the first found since we for reverse
+      // iteration).
+      if (!R.second)
+        ToBeRemoved.push_back(&DPV);
+      continue;
+    }
+    // Sequence with consecutive dbg.value instrs ended. Clear the map to
+    // restart identifying redundant instructions if case we find another
+    // dbg.value sequence.
+    VariableSet.clear();
+  }
+
+  for (auto &DPV : ToBeRemoved)
+    DPV->eraseFromParent();
+
+  return !ToBeRemoved.empty();
+}
+
 static bool removeRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
+  if (BB->IsNewDbgInfoFormat)
+    return DPValuesRemoveRedundantDbgInstrsUsingBackwardScan(BB);
+
   SmallVector<DbgValueInst *, 8> ToBeRemoved;
   SmallDenseSet<DebugVariable> VariableSet;
   for (auto &I : reverse(*BB)) {
@@ -440,7 +471,39 @@ static bool removeRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
 ///
 /// Possible improvements:
 /// - Keep track of non-overlapping fragments.
+static bool DPValuesRemoveRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
+  SmallVector<DPValue *, 8> ToBeRemoved;
+  DenseMap<DebugVariable, std::pair<SmallVector<Value *, 4>, DIExpression *>>
+      VariableMap;
+  for (auto &I : *BB) {
+    for (DPValue &DPV : I.getDbgValueRange()) {
+      DebugVariable Key(DPV.getVariable(),
+                        std::nullopt,
+                        DPV.getDebugLoc()->getInlinedAt());
+      auto VMI = VariableMap.find(Key);
+      // Update the map if we found a new value/expression describing the
+      // variable, or if the variable wasn't mapped already.
+      SmallVector<Value *, 4> Values(DPV.location_ops());
+      if (VMI == VariableMap.end() || VMI->second.first != Values ||
+          VMI->second.second != DPV.getExpression()) {
+        VariableMap[Key] = {Values, DPV.getExpression()};
+        continue;
+      }
+      // Found an identical mapping. Remember the instruction for later removal.
+      ToBeRemoved.push_back(&DPV);
+    }
+  }
+
+  for (auto *DPV : ToBeRemoved)
+    DPV->eraseFromParent();
+
+  return !ToBeRemoved.empty();
+}
+
 static bool removeRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
+  if (BB->IsNewDbgInfoFormat)
+    return DPValuesRemoveRedundantDbgInstrsUsingForwardScan(BB);
+
   SmallVector<DbgValueInst *, 8> ToBeRemoved;
   DenseMap<DebugVariable, std::pair<SmallVector<Value *, 4>, DIExpression *>>
       VariableMap;
diff --git a/llvm/test/Transforms/DCE/dbg-value-removal.ll b/llvm/test/Transforms/DCE/dbg-value-removal.ll
index 3506546fca28e5f..f8f01120d0a0692 100644
--- a/llvm/test/Transforms/DCE/dbg-value-removal.ll
+++ b/llvm/test/Transforms/DCE/dbg-value-removal.ll
@@ -1,5 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -S -passes=redundant-dbg-inst-elim | FileCheck %s
+; RUN: opt < %s -S -passes=redundant-dbg-inst-elim --try-experimental-debuginfo-iterators | FileCheck %s
 
 ; All dbg.value with location "!dbg !19" are redundant in the input.
 ; FIXME: We do not handle non-overlapping/overlapping fragments perfectly yet.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 57dd23bc0a2f7b4f7b68162923b3267c1f303de9 72ce6d13e71ef27a8be63f2e309375c5a44a40ad -- llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index b64029f256..06347cc0fd 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -387,8 +387,7 @@ static bool DPValuesRemoveRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
   SmallDenseSet<DebugVariable> VariableSet;
   for (auto &I : reverse(*BB)) {
     for (DPValue &DPV : reverse(I.getDbgValueRange())) {
-      DebugVariable Key(DPV.getVariable(),
-                        DPV.getExpression(),
+      DebugVariable Key(DPV.getVariable(), DPV.getExpression(),
                         DPV.getDebugLoc()->getInlinedAt());
       auto R = VariableSet.insert(Key);
       // If the same variable fragment is described more than once it is enough
@@ -477,8 +476,7 @@ static bool DPValuesRemoveRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
       VariableMap;
   for (auto &I : *BB) {
     for (DPValue &DPV : I.getDbgValueRange()) {
-      DebugVariable Key(DPV.getVariable(),
-                        std::nullopt,
+      DebugVariable Key(DPV.getVariable(), std::nullopt,
                         DPV.getDebugLoc()->getInlinedAt());
       auto VMI = VariableMap.find(Key);
       // Update the map if we found a new value/expression describing the

[OCHyams]

LGTM (clang-format pls)

[OCHyams]

// FIXME: Add assignment-tracking support (see removeRedundantDbgInstrsUsingBackwardScan).?

(maybe not worth the note if it's not been done everywhere)

[OCHyams]

If you want to keep the code as similar as possible to the dbg.value version then ignore this, but we could use DebugVariableAggregate here now instead of DebugVariable.

[jmorse]

As far as I can see, the "Aggregate" version only happens down dbg.assign code paths? (Which I'm ignoring for the now)

[OCHyams]

It's nothing to do with dbg.assigns - it's just a specialised DebugVariable without a FragmentInfo component, to indicate that it refers to the whole variable. The DebugVariable above is created with a std::nullopt FragmentInfo unconditionally, which is essentially the same thing as a DebugVariableAggregate. It was more a matter of "should we update this code while we're there?" as the change is NFC. No is still a fine answer, I just thought it should be clarified.

[jmorse]

Ah cool, for some reason my mind removed the "then ignore this" wording and thought it was a direct suggestion. Indeed, IMO keeping identical-ness is more important at this stage.

[jmorse]

Merged in 6d23aaa