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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
 
----------------
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)


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

https://reviews.llvm.org/D78369