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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81198