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


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

https://reviews.llvm.org/D78369