[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:29
+
+A transformation must preserve the debug location of an instruction if the
+instruction either remains in its basic block, or if its basic block is folded
----------------
vsk wrote:
> dblaikie wrote:
> > Is this kind of "must" language consistent with the rest of this document? In this case it's an aspirational, sort of "this is ideal/what we're striving for" rather than "if you don't do this it'll fail the verifier/break", etc?
> This is aspirational -- would "should" be more appropriate?
"should" sounds right to me - but open  to other folks preferences/ideas about how this should be framed


================
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:
> > 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81198