[PATCH] D78369: [DebugInfo] Reduce SalvageDebugInfo() functions

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

Orlando added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:359
 /// Returns true if any debug users were updated.
 bool salvageDebugInfo(Instruction &I);
 
----------------
dblaikie wrote:
> Orlando wrote:
> > dblaikie wrote:
> > > chrisjackson wrote:
> > > > chrisjackson wrote:
> > > > > jmorse wrote:
> > > > > > vsk wrote:
> > > > > > > vsk wrote:
> > > > > > > > aprantl wrote:
> > > > > > > > > Should we rename this to `updateDebugInfoForDeletedInsn()` or some variant of that to make it more clear that it isn't optional to call this method?
> > > > > > > > I don't think the current name is unclear (I'm not opposed to changing it, just don't see a strong need to right now).
> > > > > > > > 
> > > > > > > > However, what /does/ seem missing is some way to enforce that this method is called. I like @alok's approach in D70419: wdyt of just making `Instruction::eraseFromParent()` call `Instruction::salvageDebugInfo()`? (This would be for a follow-up, my preference would be to consolidate APIs here first.)
> > > > > > > Ah, one issue with this is that the common deletion idiom is: "I.RAUW(replacement); I.eraseFromParent();" -- so if the salageDebugInfo operation occurs in eraseFromParent, in many cases it's too late, because the debug uses are gone.
> > > > > > I've no opinion on changing the name; for the "RAUW; Erase" deletion idiom I don't think that's a problem, it just means those locations were saved by the RAUW instead of through salvaging. A salvage call in eraseFromParent would be highly useful in catching the more complicated circumstances, for example I believe that during inlining large numbers of instructions are pruned for being unused, without ever being RAUW'd. 
> > > > > Yes this is something I'd like to address too.
> > > > I'm not sure that a method name is a good way of reflecting its necessity. 
> > > I'll +1 @aprantl's comment - it sounds like at least from the comment the previous version of this function was "best effort" - if you didn't call it you just got worse debug info. But the new behavior seems like it might break (result in incorrect debug info) if you don't call this function? Is that the case? (if so, then a new name seems justified)
> > 
> > > it sounds like at least from the comment the previous version of this
> > > function was "best effort" - if you didn't call it you just got worse debug
> > >info.
> > 
> > This is only true if `replaceDbgUsesWithUndef` was being called where
> > appropriate. With D80264 we do actually get this for free in a lot of cases,
> > but there are still times where we want to undef a dbg.value without deleting
> > the instruction. E.g. When sinking an instruction with a corresponding
> > dbg.value, we want to leave behind a salvage-or-undef, and sink a clone
> > of the original dbg.value to the new instruction position.
> > 
> >> it sounds like at least from the comment the previous version of this
> function was "best effort" - if you didn't call it you just got worse debug
> info.
> >
> > This is only true if replaceDbgUsesWithUndef was being called where
> appropriate. 
> 
> Sorry, not sure I understand - you mean previously you had to call one of either replaceDbgUsesWithUndef (not anymore, since D80264, now that's implicit) or salvageDebugInfo - and now you have to always call salvageDebugInfo ?
> 
> (maybe I shuold just bow out of this review - I haven't gotten very involved in LLVM's debug info variable location tracking, so I don't have a lot of context for this - just wanted to express some agreement with @aprantl's perspective there)
Ah sorry for being unclear, I hope this helps. I don't have a strong opinion on the name change.

AFAIK it is correct to call `replaceDbgUsesWithUndef` if `salvageDebugInfo` failed (returns false), and not doing so can lead to wrong debug-info by allowing previous dbg.value definitions to run on for too long. This patch changes `salvageDebugInfo` to call `replaceDbgUsesWithUndef` itself if it fails to salvage. Because `replaceDbgUsesWithUndef` is now called automatically when we delete an instruction (D80264), not calling `salvageDebugInfo` just reduces coverage, but doesn't produce incorrect debug-info when deleting instructions.

> But the new behavior seems like it might break (result in incorrect debug info) if you don't call this function? Is that the case? (if so, then a new name seems justified)

So I think the answer to this is no, now that we have D80264.


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

https://reviews.llvm.org/D78369