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

[David Stenberg via Phabricator via llvm-commits]

dstenb 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);
----------------
Nit: check -> Check, and a dot at the end of the instruction.


================
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;
----------------
Can we move this into a `isDbgValueWithValue(DVI, I, DIVar, DIExpr)` helper or similar, and also use that for the "before I" check?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1256
+          DVI->getExpression() == DIExpr)
+        return true;
+  }
----------------
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.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74030