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

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 9 inline comments as done.
alok added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1239
   // not inserting the same dbg.value intrinsic over and over.
+  // check if the same dbg.value already exists before I
   BasicBlock::InstListType::iterator PrevI(I);
----------------
dstenb wrote:
> dstenb wrote:
> > Nit: check -> Check, and a dot at the end of the instruction.
> s/instruction/comment/
Thanks a lot for your comment, i shall update patch for this.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1254-1255
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(NextI))
+      if (DVI->getValue() == I && DVI->getVariable() == DIVar &&
+          DVI->getExpression() == DIExpr)
+        return true;
----------------
dstenb wrote:
> Can we move this into a `isDbgValueWithValue(DVI, I, DIVar, DIExpr)` helper or similar, and also use that for the "before I" check?
Sure, I shall do that in next revision.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1256
+          DVI->getExpression() == DIExpr)
+        return true;
+  }
----------------
dstenb wrote:
> I wonder if we also need to check that the locations' getInlinedAt() are identical. Otherwise, I think this may result in us not emitting a dbg.value for a different inlined instance of the variable. I'm not sure if we can land in such a case for a load or store in practice though.
> 
> (Same goes for the "before I" check.)
Thanks for your comment, I shall include that in next revision.


================
Comment at: llvm/test/DebugInfo/duplicate_dbgvalue.ll:1
+; RUN: opt -O2 -S -o - < %s | FileCheck %s
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74030