[PATCH] D81198: [docs] Specify rules for updating debug locations

Tue Jun 9 10:58:24 PDT 2020

rob.lougher added inline comments.

================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:62-64
+A transformation must merge instruction locations if it replaces multiple
+instructions with a single merged instruction, *and* the merged instruction is
+in a different block than at least one of the instructions-to-be-merged. The
----------------
probinson wrote:
> rob.lougher wrote:
> > rob.lougher wrote:
> > > probinson wrote:
> > > > dblaikie wrote:
> > > > > vsk wrote:
> > > > > > dblaikie wrote:
> > > > > > > vsk wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > Should this be "or" rather than "and"? If you merge two instructions even in the same BB, I think the goal is you should use a merged location, right?
> > > > > > > > I'm of two minds on this one. The applicable (hypothetical) example here is "(A * B) + C => llvm.fma.f32(A, B, C)".
> > > > > > > > 
> > > > > > > > If the part about "being in a different block" is kept, then the location of the "fma" instruction should be determined by location cascade. That location has a realistic shot at mapping back to some real (if somewhat arbitrary) source line.
> > > > > > > > 
> > > > > > > > If get rid of the "being in a different block" language, the location of the "fma" instruction should be artificial (line 0).
> > > > > > > > 
> > > > > > > > It seems like both options suit the interactive debugging and crash triage use cases. But maybe location cascade would be a better fit for SamplePGO, if that needs to find a "real" line number to increment the right basic block execution count? It'd be helpful for anyone familiar with SamplePGO to express some opinion (cc @danielcdh).
> > > > > > > Not sure I follow the fma example.
> > > > > > > 
> > > > > > > Let me rephrase the words in the document to see if we're both understanding them the same way: "IF multiple instructions are merged and the merged location is in a different block than one of the inputs - you /should/ (must, whatever) merge" - the implication is then if you have multiple instructions that don't change blocks, the location should /not/ be merged?
> > > > > > > 
> > > > > > > That doesn't sound like what I'd expect - in the fma example, if the * and - were from the same basic block, I'd still expect to merge the location rather than dropping or zeroing the location. So that scope information was preserved (eg: if * and - came from the same inline function, then the fma should still be attributed to that inline function).
> > > > > > Re: "the implication is then if you have multiple instructions that don't change blocks, the location should /not/ be merged" -- yep, we're on the same page about the proposed rule. Your point about expecting the right inline scope on merged instructions is compelling though. I'll fix this up.
> > > > > > 
> > > > > > Part of my confusion here was over how exactly SamplePGO works: can it correctly attribute a sample of a line 0 location to the scope it points to? Maybe that's moot, though, since relying on location cascade to assign locations to merged instructions could be more misleading than helpful.
> > > > > > Part of my confusion here was over how exactly SamplePGO works: 
> > > > > 
> > > > > I don't actually know how it works.
> > > > > 
> > > > > > can it correctly attribute a sample of a line 0 location to the scope it points to? 
> > > > > 
> > > > > In theory I think such samples could be used, but fairly likely they aren't used.
> > > > > 
> > > > > (you could imagine a sample based profiler could look at all source lines within a sampled basic block - and mark all those source lines as sampled)
> > > > I believe that (at least how we use it in Sony) SPGO doesn't use a line-0 location, but snoops around looking for a real source location in the same block.  Jeremy will poke a couple people who actually worked on it to try to get a more definitive answer tomorrow (they're in the UK).
> > > When sampling an executable, the "hits" are addresses which must be mapped back to source locations using debug line info (i.e. addr2line). Any transform that moves or merges instructions without updating the debug location can therefore affect the quality of the sample. The standard example is tail-merging (which in fact was the first optimisation we fixed).
> > > 
> > > Imagine we have an if-then-else, where the "then" and "else" blocks end with the same instruction(s). Tail-merge will common the instructions in the successor block.
> > > 
> > > For block-reordering, we want to know if one side of the if-then-else is executed much more than the other. But if tail-merge has retained the original debug locations in the merged tail we will get an inaccurate sample. If the "then" location is used, hits on the common instructions will be attributed to the "then" block which is wrong. Likewise if the "else" locations were used. The location of the tail is ambiguous.
> > > 
> > > To fix this we introduced the "getMergedLocation" call. The initial version was very simple. If the locations were the same, either can be used. But if the locations are different, we can't give a location and an empty ("unknown") location is used.
> > > 
> > > At some point, this "unknown" location ends up generating a DWARF line-0 record. Now this is the bit I didn't directly work on so I don't know where it occurs. My memory is that the work was done by Paul, and that an "unknown" location at the start of a basic block forced a line-0. This did exactly what we wanted, and we rewrote our initial patch to tail-merge that explicitly created a line-0 location.
> > P.S. We did a presentation of this work at EuroLLVM 2017 (https://www.youtube.com/watch?v=ceCEXnuWdmo) which you can watch for more examples.
> Rob, I think the most relevant question is:  If there's a sample hit on an address that maps to line 0, what does SPGO do with it?  Throw it away?  Rummage around for a non-zero location?
This is handled by AutoFDO (which reads a "raw" sample and converts it into a sample with lines relative to the start of the function). It's 4 years since I hacked that code so I wanted to check to make sure.

Anyway, the answer is that samples for addresses that map to line 0 are simply thrown away...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81198