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

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 4 inline comments as done.
alok added a comment.

In D74030#1888527 <https://reviews.llvm.org/D74030#1888527>, @vsk wrote:

> @alok I had a quick stab at this to see what it would take, and the result turned out to be relatively clean: https://github.com/vedantk/llvm-project/commit/85d7fe06810fa7f6166508be73eef80f676b0fc6.diff. This handles the situation in your 'duplicate_dbgvalue.ll' test case. Lmk what you think. Feel free to commandeer the patch if you'd like, or if you don't have the bandwidth for that I can kick off a fresh review. Thanks!


Thanks for your comment/suggestion. I would update the patch to call RemoveRedundantDbgInstrs as you suggested. I would also limit the check before inserting dbg.value instruction to immediate next instruction in place of linear forward search considering the performance impact. This will enable us to avoid inserting consecutive duplicates, and remove duplicates if are far apart. Thanks again for your suggestion.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1268
+      if (isDbgValueMatching(DVI, I, DIVar, DIExpr, DbgLoc))
         return true;
   }
----------------
jmorse wrote:
> alok wrote:
> > aprantl wrote:
> > > Is checking the first instruction after going to be good enough in the general case? Optimized code often has dozens of dbg.values in a row, all pointing to the same SSA value. (For example C++ code will often have many inlines "this" variables from inlined trivial getters).
> > Thanks for pointing this out. Even I though this and ignored. Yes it will be better to iterate through next instructions. I shall be updating patch for that. 
> Shouldn't this loop terminate when a non-debug-value instruction is seen? Otherwise it's not checking for the presence of a dbg.value for a single load/store, but instead a whole block.
Thanks for your comment. Looking at the possible performance impact, I am going to limit the scan to the immediate next instruction. 


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1259
     if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(PrevI))
-      if (DVI->getValue() == I->getOperand(0) &&
-          DVI->getVariable() == DIVar &&
-          DVI->getExpression() == DIExpr)
+      if (isDbgValueMatching(DVI, I->getOperand(0), DIVar, DIExpr, DbgLoc))
+        return true;
----------------
aprantl wrote:
> Orlando wrote:
> > @aprantl said:
> > >Is checking the first instruction after going to be good enough in the general case?
> > 
> > In this case shouldn't the 'already exists before I' check also be a backwards scan (through consecutive dbg.values)?
> Yeah, and doing a linear scan each time might be prohibitively expensive (i.e., quadratic). If we're implementing this we should take a look at the performance of compiling an optimized clang and an optimizes asanified clang to make sure this isn't a problem. 
Thanks for your comment. Yes I too feel that linear scan doesnt look like a good idea and it should be limited to check intimidate previous/next instruction that is where it has the greatest probability to exist.


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

https://reviews.llvm.org/D74030