[PATCH] D87233: [POC][DebugInfo] Use entry values within IR

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

In D87233#2288809 <https://reviews.llvm.org/D87233#2288809>, @dstenb wrote:

> Sorry for a piecemeal review from my part!

No problem, the tempo works for me. :) Thanks!



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1612
+  for (auto &I : EntryBB) {
+    LLVM_DEBUG(llvm::dbgs() << "  Investigating: "; I.dump(););
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
----------------
dstenb wrote:
> Perhaps this is too verbose?
No problem, I'll remove this.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1613
+    LLVM_DEBUG(llvm::dbgs() << "  Investigating: "; I.dump(););
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
+      if (DVI->getVariable() != Var)
----------------
dstenb wrote:
> Maybe make this an early continue?
> 
> ```
> DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I);
> if (!DVI)
>   continue;
> 
Sure.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1622-1623
+      // Do not consider variables from inlined functions.
+      if (DVI->getDebugLoc()->getInlinedAt())
+        return nullptr;
+
----------------
dstenb wrote:
> I might be misunderstanding something here, but shouldn't we want to still search for the debug value even though we have encountered an inline variant of the same variable? I guess this is something that is not really common but still.
> 
> If so, this needs to be moved above the `getNumElements()` check.
Nice catch, thanks!


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

https://reviews.llvm.org/D87233