[PATCH] D59272: [DebugInfo] Select debug intrinsic line-numbers more carefully when promoting dbg.declare

Mon Mar 25 08:07:23 PDT 2019

bjope added a comment.

In D59272#1441475 <https://reviews.llvm.org/D59272#1441475>, @jmorse wrote:

> Hi,
>
> In D59272#1439778 <https://reviews.llvm.org/D59272#1439778>, @bjope wrote:
>
> > Isn't there a risk that this (partially) is hiding bugs in some other passes, where the DebugLoc is picked from the wrong place. I guess that either it should be picked from an adjacent non-meta instruction. Or it should be set to unknown.
> >  With this patch we only eliminate the risk that the faulty passes are picking an "incorrect" DebugLoc from a dbg.value intrinsic (or DBG_VALUE instruction). But the "faulty" pass could still pick incorrect DebugLoc from other instructions, right? Or is it always correct to take the DebugLoc from all other non-dbg.value instructions, including all other kinds of meta-instructions?
> >
> > Might be hard to find all such bugs. But with your known examples it shouldn't be too hard to find at least some places in opt/llc where the DebugLoc from the dbg.value is infecting some other instruction. Then perhaps we want to land this patch as well, just do avoid some not-yet-detected problems.
>
>
> All true -- this patch can be characterised as simply reducing the risk of leaking definitely-wrong line numbers into non-meta IR instructions, which I think has some value.
>
> Quickly testing what happens if IRBuilder is adjusted to not pick take DebugLocs from debug-intrinsics [0], roughly 750 of the 900 entries this patch changes, are also changed, possibly to different lines though. (I'll upload this trivial change sometime soon too).
>
> That's likely the bulk of the leaking line numbers; while the rest could be hunted down, they're likely to be scattered through many places, so IMO this patch is the most time-efficient solution going forwards.
>
> [0] https://github.com/llvm-mirror/llvm/blob/e3696113b639c8bf0b72d6c27dd76d6fdd8ebf61/include/llvm/IR/IRBuilder.h#L137

Ok, great. I'm happy with that explanation.

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

https://reviews.llvm.org/D59272