[PATCH] D80052: [docs] Sketch outline for HowToUpdateDebugInfo.rst

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 11 inline comments as done.
vsk added a comment.

Sorry for the delay. I'll share an update soon. I'll plan to land this by EOD next Monday (PST) if there aren't any objections, so we can parallelize work on this.



================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:2
+========================
+How to Update Debug Info
+========================
----------------
aprantl wrote:
> `How to update Debug Info after a code transformation`
> ?
I humbly submit "s/after a code transformation/: A Guide for LLVM Pass Authors/" to make the target audience super-explicit.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:21
+
+IR-level transformations
+========================
----------------
djtodoro wrote:
> A TODO on changing DIMeatadata (such as introducing a DIFlag etc) by describing bitcode backward compatibility etc. ?
I think we should spin up a separate document to describe how to hack on llvm's debug info facilities/representation. That way, we can keep this doc focused on what llvm pass authors need to know.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:27
+
+When an ``Instruction`` is deleted, its debug uses change to ``undef``.  This
+is a loss of debug info: the value of a one or more source variables becomes
----------------
dexonsmith wrote:
> aprantl wrote:
> > vsk wrote:
> > > TWeaver wrote:
> > > > Hi and thanks for this!
> > > > 
> > > > I'm not sure this line is actually correct.
> > > > 
> > > > It's my assumption that when a value pointed at by an intrinsic is deleted, the intrinsic is left with a valueless ValueAsMetaData() operand.
> > > > 
> > > > This empty ValueAsMetaData points at an empty metadata node, thus leaving us with an 'empty' intrinsic.
> > > > 
> > > > These empty intrinsics are usually removed from the program by later passes that clean up or remove 'junk' from the instruction stream.
> > > > 
> > > > My reading of this line gives me the impression that if I delete a value pointed to by an intrinsic, the intrinsic is auto-magically fixed up to be undef.
> > > > 
> > > > This may not have been your intent but it's how it reads to me.
> > > > 
> > > > I've fixed a bug in the Reassociate pass related to this issue in:
> > > > 
> > > > https://reviews.llvm.org/rG41c3f76dcd0daeec60eddfcb56008a31ad6e8738
> > > Thanks for pointing this out. This seems bad.
> > > 
> > > A valueless dbg.value is treated as trivially dead (see `wouldInstructionBeTriviallyDead`). So unless specific care is taken to call `salvageDebugInfoOrMarkUndef`, deleting an Instruction -- and then running any instruction simplification helper -- results in inaccurate debug info.
> > > 
> > > @aprantl @dexonsmith can we change Instruction deletion s.t. any ValueAsMetadata nodes pointing to the dying value change to undef?
> > Short answer: I don't know yet, we'll have to figure this out.
> > @aprantl @dexonsmith can we change Instruction deletion s.t. any ValueAsMetadata nodes pointing to the dying value change to undef?
> > 
> 
> Sounds like a great idea to me (although I haven't looked at the IR implications of that recently).
Great, this is done as of D80264.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:232
+   be precise enough), moving the test to its own file is preferred.
+
+MIR-level transformations
----------------
jmorse wrote:
> djtodoro wrote:
> > vsk wrote:
> > > aprantl wrote:
> > > > Do we want to add a section on SelectionDAG & GlobalISel, DAGCombine, etc?
> > > The info about MIR should generally carry over to GIsel. This probably does need a separate section for SelectionDAG, but I'm not sure how to structure that, so I'll leave it as TODO as well.
> > I guess we can add TODOs for the sections on VirtRegRewriter/LiveDebugVariables; LiveDebugValues; AsmPrinter(DwarfDebug;DwarfExpression); as well.
> My feeling is that the debug specific passes are best documented in SourceLevelDebugging.rst instead -- with this document focused on how optimisation writers should use LLVM APIs to preserve debug info in their passes.
> 
> The isel passes kind of fall into both those categories -- there's debug updating occurring, but lots of unrelated implementation details too.
+1 to @jmorse's comment here.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:264-266
+The ``debugify`` utility described in
+:ref:`Mutation testing for IR-level transformations` can be used for MIR-level
+transformations as well.
----------------
dsanders wrote:
> This is slightly misleading because the MIR one is called `mir-debugify` and we don't have a check pass, we just strip it
I've edited this to make it less misleading (and call out that there's no equivalent check-debugify pass).


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:277-282
+To run MIRDebugify, simply add ``mir-debugify`` to your ``llc`` invocation,
+like:
+
+.. code-block:: bash
+
+  $ llc -run-pass=mir-debugify ...
----------------
dsanders wrote:
> This will only run `mir-debugify` and not the normal pipeline. If you want to run it with a specific pass you need `-run-pass=mir-debugify,other-pass`.
> 
> To run it as part of the normal pipeline there's `-debugify-and-strip-all-safe` which can be combined with `-start-before`/`-start-after`. There's currently no way to run it once and then resume the normal pipeline without running it between every pass.
Thanks, I've tried to fold this info in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80052