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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This generally sounds fine to me, I have some high level comments: if this functionality is likely to be repeated elsewhere, it might be better to use the DebugVariable class in DebugInfoMetadata.h. It'd be more concise to compare variable identities by equality of DebugVariable objects rather than by individual components. (Not necessary for this patch, but worth keeping in mind if there are similar upcoming patches).

Repeatedly scanning through all dbg.values after an instruction could be expensive when there are a lot of them, for example in asan builds, which has bitten me in the past. I'd recommend checking the performance impact of a small asan build; it might also be that in the specific circumstance that LdStHasDebugValue targets, it's sufficient for a shallow test of a single dbg.value.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1268
+      if (isDbgValueMatching(DVI, I, DIVar, DIExpr, DbgLoc))
         return true;
   }
----------------
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.


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

https://reviews.llvm.org/D74030