[PATCH] D100845: [Debugify][Original DI] Test preservation of original debug var intrinsics in optimizations

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

> I am seeing some false positives for sure (most of these artifital vars locs are being removed after some "constant propagation" passes). We definitely need to investigate how to remove these false positives here (for both, instr and var locations).

I guess that can all fall under "follow up work" too? I think it is worth mentioning the caveat that there are known false positive cases in https://llvm.org/docs/HowToUpdateDebugInfo.html either way (for both source locations and this new debug intrinsic checking).

Bringing the undef discussion out from the inline comments to reduce clutter:

>> @Orlando  said:
>> While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.
>
> @djtodoro said:
> Hmmm... it does not affect the correctness, but it does affect completeness (e.g. in optimized code, variable should be covered for the places it is alive, but in many cases it is impossible). How are we going to find cases where we have missed to call `salvageDebugInfo()`?
> In addition, we can add some checking levels -- e.g. there could be level that doesn't consider "undef" locations, which will reduce the number of false positives, for sure...

The best thing to do here probably depends on the goals of the tool. Is it trying to find passes that introduce correctness bugs or find passes that reduce coverage unnecessarily? If the answer is "both" then I agree that a checking-level option is probably a good way forward to help reduce the noise when looking for correctness bugs.

On the whole this SGTM but I haven't contributed to or recently used debugify so I would like see if anyone else has any comments on the approach in general before reviewing the code changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100845