[PATCH] D74030: [DebugInfo] Avoid generating duplicate llvm.dbg.value

[David Stenberg via Phabricator via llvm-commits]

dstenb added a comment.

In D74030#1868655 <https://reviews.llvm.org/D74030#1868655>, @alok wrote:

> Hi David (@dstenb), I have incorporated all your comments. I have also added you as reviewer. Kindly give your feedback.


Hi, sorry for the delay! I have some more minor comments.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1233
 
-/// See if there is a dbg.value intrinsic for DIVar before I.
+/// See if dbg.value intrinsic DVI matches
+static bool isDbgValueMatches(DbgValueInst *DVI, Instruction *I,
----------------
[...] matches //the given operands.//

(Or something along those lines).


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1234
+/// See if dbg.value intrinsic DVI matches
+static bool isDbgValueMatches(DbgValueInst *DVI, Instruction *I,
+                              DILocalVariable *DIVar, DIExpression *DIExpr,
----------------
dstenb wrote:
> Can these pointer variables be made const?
`isDbgValueMatches` reads a bit awkward to me. Perhaps `isDbgValueMatching`?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1234-1235
+/// See if dbg.value intrinsic DVI matches
+static bool isDbgValueMatches(DbgValueInst *DVI, Instruction *I,
+                              DILocalVariable *DIVar, DIExpression *DIExpr,
+                              DebugLoc DbgLoc) {
----------------
Can these pointer variables be made const?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1242
+    if (DVI->getVariable() == DIVar && DVI->getExpression() == DIExpr &&
+        (DVI->getValue() == I->getOperand(0) || DVI->getValue() == I))
+      return true;
----------------
To make this function more general, perhaps we should make it take a `Value` instead of an `Instruction`, and specify `I` or `I->getOperand(0)` at the call sites?


================
Comment at: llvm/test/DebugInfo/duplicate_dbgvalue.ll:1
+; RUN: opt -O2 -S -o - < %s | FileCheck %s
+
----------------
alok wrote:
> dstenb wrote:
> > Unless I'm overlooking something, I think that this should be possible to make a single-pass test, which is preferable since it makes the test more stable, and makes the changes easier to review.
> > 
> > A suitable single-pass test case can often be found by looking at the opt output with `-print-before-all -print-after-all -print-module-scope` added.
> Thanks for your comment. I shall incorporate this in next revision.
Thanks!


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

https://reviews.llvm.org/D74030