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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D74030#1891319 <https://reviews.llvm.org/D74030#1891319>, @alok wrote:

> In D74030#1889750 <https://reviews.llvm.org/D74030#1889750>, @vsk wrote:
>
> > In D74030#1888655 <https://reviews.llvm.org/D74030#1888655>, @alok wrote:
> >
> > > Now,
> > >
> > > 1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)
> >
> >
> > Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.
>
>
> Yes for me also it seems a bit difficult decision. There were few reasons which made me think to keep 1)
>
>   a) It looks clearer to avoid generating than first let it generate and later delete when there is no performance penalty. In case of avoiding generation of dbg.value for same load/store again and again there is a pattern (immediate previous/next instruction in question).


I'm not sure I understand how the IR could look clearer. It shouldn't make any difference if the redundant instructions are eliminated after all insertions occur. Unless there's some kind of significant compile time reduction to be had by keeping LdStHasDebugValue, I think we should get rid of it, as it'd just be adding unnecessary complexity.

>   b) There is existing functionality for avoiding generation in function named "LdStHasDebugValue", though the name suggests covering both load and store both but it had handling only for store, adding the case for load justifies its name and completes the functionality. Either we should remove this function completely or complete it.

Agreed! Strong +1 to this, it's just that I suspect deleting LdStHasDebugValue is the simpler way to go.


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

https://reviews.llvm.org/D74030