[PATCH] D60831: [DebugInfo at O2][LoopVectorize] pr39024: Vectorized code linenos step through loop even after completion

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In D60831#1472518 <https://reviews.llvm.org/D60831#1472518>, @vsk wrote:

> In D60831#1472494 <https://reviews.llvm.org/D60831#1472494>, @jmellorcrummey wrote:
>
> > In D60831#1472310 <https://reviews.llvm.org/D60831#1472310>, @gbedwell wrote:
> >
> > > In D60831#1472303 <https://reviews.llvm.org/D60831#1472303>, @aprantl wrote:
> > >
> > > > In D60831#1472296 <https://reviews.llvm.org/D60831#1472296>, @jmellorcrummey wrote:
> > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > > > > As the lead of a project building profiling tools, I am strongly against having any instructions map to line 0.
> > > >
> > > > This is probably not what you meant, but for completeness I feel like I should point out that there are many legitimate situations where LLVM generates a line 0 location. The most prominent example is instruction merging: Since both LLVM IR and DWARF currently require each PC address to map to exactly one source location, LLVM's will insert a line 0 location when it merges two instructions with distinct source locations. I can't speak for profiling, but at least on the debugger side, the consensus is that potentially misleading information is worse than no information, because if there is no way to distinguish "always correct" from "maybe correct" information, the user can't trust any information.
> > >
> >
> >
> > When merging happens, I don't see why mapping to 0 is better than attributing it to one of a set of locations that a fused operation came from. I see that as representative of reality.
> >
> > I would prefer a rule stating that if two operations are merged, always pick the lexicographically least (file, line number) pair. In the case of mappings for merged instructions, I see either mapping as "correct".
>
>
> I don't see things this way. Arbitrarily picking a location can result in a false execution history being presented to the user. Line 0 works a lot better imo, because it a) doesn't actively mislead and b) preserves inline scope.
>
>      
>
> > 
> > 
> >> FWIW, the motivating case for the introduction of getMergedLocation, which generally handles the insertion of line 0 locations in this case was to improve the performance of profile-guided optimized code when using a sampling profiler tool (see http://llvm.org/devmtg/2017-03//assets/slides/delivering_sample_based_pgo_for_playstation_r_4.pdf ).
> > 
> > I looked at the slides. The PGO is nice work. The slide about getMergedLocation describes the rationale for mapping to line 0 rather than another line mapping if the line mappings don't agree. I don't see it wrong to simply choose one.


A good example for why arbitrarily picking one location during merging is when the two locations are coming from different inlined instances of different functions (or perhaps even worse: two inlined instances of the same function). I would assume that even in profiling a wrong backtrace would invalidate or render untrustworthy large parts of any analysis being done one this data.


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

https://reviews.llvm.org/D60831