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

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson marked 4 inline comments as done.
chrisjackson added inline comments.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:359
 /// Returns true if any debug users were updated.
 bool salvageDebugInfo(Instruction &I);
 
----------------
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.


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:359
 /// Returns true if any debug users were updated.
 bool salvageDebugInfo(Instruction &I);
 
----------------
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. 


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

https://reviews.llvm.org/D78369