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

[Vedant Kumar via Phabricator via llvm-commits]

vsk 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81198