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

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked an inline comment as done.
vsk added a subscriber: danielcdh.
vsk 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
----------------
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?


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


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:86-87
+
+* Speculative execution of *different* instructions from both sides of a CFG
+  diamond. The rule for :ref:`dropping locations<WhenToDropLocation>` applies.
+
----------------
echristo wrote:
> Bit more clarification here?
How about: "Converting an if-then-else CFG diamond into a select. Preserving the debug locations of speculated instructions can make it seem like a condition is true when it's not (or vice versa), which can lead to a confusing single-stepping experience. The rule for :ref:`dropping locations<WhenToDropLocation>` should apply here." ?


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:121-123
+When creating a new instruction that doesn't map back to a source line, use an
+artificial line 0 location instead of setting no location at all. The API to
+use for this is ``DILocation::get()``.
----------------
dblaikie wrote:
> Might be worth a separate header - not sure how consistently we do this or how problematic it is if this isn't done.
That's a good point, e.g. I don't think the ASan pass bothers with setting line 0 locations. I'll try to split this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81198