[PATCH] D105279: [RemoveRedundantDebugValues] Introduce a MIR pass that removes redundant DBG_VALUEs

Fri Jul 2 03:23:56 PDT 2021

Orlando added a comment.

LGTM with a few inline nits (mostly about comments). I'd prefer to wait a little and see if anyone else has comments before hitting Accept. Note to other reviewers: there is a discussion about the stats/impact of this patch-set on D105280 <https://reviews.llvm.org/D105280>.

================
Comment at: llvm/docs/SourceLevelDebugging.rst:773-775
+the undefined location. After the LiveDebugVariables is being done, we can
+end up having redundant DBG_VALUEs (because of virtual register rewritting),
+so the RemoveRedundantDebugValues pass removes all such DBG_VALUEs.
----------------
nit: this feels maybe a bit clunky? Here is a possible alternative.

================
Comment at: llvm/lib/CodeGen/RemoveRedundantDebugValues.cpp:22
+///
+/// The RemoveRedundantDebugValues removes redundant DBG_VALUEs that
+/// appear in MIR after the register allocator.
----------------

================
Comment at: llvm/lib/CodeGen/RemoveRedundantDebugValues.cpp:69-72
+// This analysis aims to remove redundant DBG_VALUEs by going backward
+// in the basic block by considering the latest DBG_VALUE in a row of
+// consecutive DBG_VALUEs for the same variable as the one that is relevant
+// value representation.
----------------
nit/suggestion: possible rewording
```
// This analysis aims to remove redundant DBG_VALUEs by going backward
// in the basic block and removing all but the last DBG_VALUE for any
// given variable in a set of consecutive DBG_VALUE instructions.
```

================
Comment at: llvm/test/DebugInfo/X86/livedebugvars-avoid-duplicates.ll:4
+
+; RUN: llc -O2 %s -stop-before=livedebugvalues -o - | FileCheck %s
+
----------------
You could add `--implicit-check-not=DBG_VALUE` to `FileCheck` so that the test fails if there are unexpected DBG_VALUEs?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105279/new/

https://reviews.llvm.org/D105279