[PATCH] D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D82129#2135946 <https://reviews.llvm.org/D82129#2135946>, @Orlando wrote:

> In D82129#2134241 <https://reviews.llvm.org/D82129#2134241>, @probinson wrote:
>
> > I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest.  So I withdraw the objection on that basis.  DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions.  This all makes me sad, but that's how DWARF is at the moment.
> >
> > Is there still an open question about whether this wants to be a cleanup pass or a verifier check?  I apologize for losing track.
>
>
> I think we have ruled out a MIR/IR verifier pass, but flagging it in llvm-dwarfdump somehow would still be nice and I wrote a ticket for fixing up the --statistics (PR46575). Instead, I think the question is now whether this should happen earlier in some way to reduce the number of redundant intrinsics, as David says in his reply below.
>
> In D82129#2134609 <https://reviews.llvm.org/D82129#2134609>, @dblaikie wrote:
>
> > My take on it is that it's probably not practical to do this as a cleanup - it'd mean any time we merge debug locations, etc, we'd have to go check for isolated variable locations that have become invalid.
> >
> > (though, inversely: I worry that not cleaning up those variable locations might be a source of IR bloat and algorithmic scaling problems when the debug locations are scanned... )
>
>
> I chose to do the trimming here because I can say with confidence that it won't cause any coverage or correctness regressions. I agree that it would be nice to remove redundant intrinsics, though I'm not exactly sure what that implementation would entail without further investigation. Is anyone able to offer any insight on this? If not, I suppose it might be reasonable to attempt to do this earlier (in IR) to see if there are any problems, and compare the results. Though I won't be able to get on this for a little while myself.


I don't have any particular insight on that, no. If no one else is stepping up, this patch as-is (though I haven't reviewed the implementation in detail) seems like a reasonable improvement at least, and should be acceptable.


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

https://reviews.llvm.org/D82129