Page MenuHomePhabricator

[docs] Sketch outline for HowToUpdateDebugInfo.rst
ClosedPublic

Authored by vsk on Fri, May 15, 5:43 PM.

Details

Summary

Sketch the outline for a new document that explains how to update debug
info in various kinds of code transformations.

Some of the guidelines that belong in HowToUpdateDebugInfo.rst were in
SourceLevelDebugging.rst already under the debugify section. It seems
like the distinction between the two docs ought to be that the former is
more prescriptive, while the latter is more descriptive.

To that end I've consolidated the "how to update debug info" guidelines
which were in SourceLevelDebugging.rst into the new doc, along with the
information about using "debugify" to test transformations. Since we've
added a mir-debugify pass, I've described that as well.

Diff Detail

Event Timeline

vsk created this revision.Fri, May 15, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 15, 5:43 PM
vsk updated this revision to Diff 264400.Fri, May 15, 6:02 PM

Clean up grammar.

TWeaver marked an inline comment as done.Mon, May 18, 3:35 AM
TWeaver added a subscriber: TWeaver.
TWeaver added inline comments.
llvm/docs/HowToUpdateDebugInfo.rst
28

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

This is what we've been missing!

llvm/docs/HowToUpdateDebugInfo.rst
3

How to update Debug Info after a code transformation
?

30

reconstitute

30

unavailable, starting with the llvm.dbg.value(undef, ...).

103

TODO

Merging two instructions
-----------------------------
233

Do we want to add a section on SelectionDAG & GlobalISel, DAGCombine, etc?

vsk marked an inline comment as not done.Mon, May 18, 5:59 PM
vsk added a subscriber: dexonsmith.
vsk added inline comments.
llvm/docs/HowToUpdateDebugInfo.rst
28

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?

233

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.

djtodoro added inline comments.
llvm/docs/HowToUpdateDebugInfo.rst
22

A TODO on changing DIMeatadata (such as introducing a DIFlag etc) by describing bitcode backward compatibility etc. ?

233

I guess we can add TODOs for the sections on VirtRegRewriter/LiveDebugVariables; LiveDebugValues; AsmPrinter(DwarfDebug;DwarfExpression); as well.

This is going to be highly useful, thanks for kicking it off. Other topics we might want to cover:

  • What to do when mutating instructions -- we saw that happen in PR45889.
  • Control flow changes -- we probably want to prescribe that if one jump threads, duplicates or erases blocks, then the same sequence of debug intrinsics is seen on each program path as before. (Or otherwise, what to do in those scenarios).

Plus: how to update DebugLocs / line-numbers. These could be in their own top level section, or documented alongside the IR transforms. (I've no opinion on which).

llvm/docs/HowToUpdateDebugInfo.rst
233

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.

Thanks for working on this. I had a couple inline comments.

We should probably mention the LostDebugLocObserver that can be integrated into passes to detect location loss while a pass runs.

llvm/docs/HowToUpdateDebugInfo.rst
265–267

This is slightly misleading because the MIR one is called mir-debugify and we don't have a check pass, we just strip it

278–283

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.

aprantl accepted this revision.Thu, May 21, 8:58 AM

To avoid the perfect being the enemy of the good, I would recommend not filling in any of the TODOs in this review, and instead create separate patches for adding additional documentation. With that said, this LGTM with all outstanding feedback addressed.

This revision is now accepted and ready to land.Thu, May 21, 8:58 AM
aprantl added inline comments.Thu, May 21, 9:01 AM
llvm/docs/HowToUpdateDebugInfo.rst
28

Short answer: I don't know yet, we'll have to figure this out.

dexonsmith added inline comments.Thu, May 21, 3:39 PM
llvm/docs/HowToUpdateDebugInfo.rst
28

@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).

davide added a subscriber: davide.Thu, May 21, 4:27 PM
vsk marked 11 inline comments as done.Fri, May 29, 12:41 PM

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.

llvm/docs/HowToUpdateDebugInfo.rst
3

I humbly submit "s/after a code transformation/: A Guide for LLVM Pass Authors/" to make the target audience super-explicit.

22

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.

28

Great, this is done as of D80264.

233

+1 to @jmorse's comment here.

265–267

I've edited this to make it less misleading (and call out that there's no equivalent check-debugify pass).

278–283

Thanks, I've tried to fold this info in.

vsk updated this revision to Diff 267338.Fri, May 29, 12:42 PM
vsk marked an inline comment as done.

Incorporate suggestions from reviewers.

vsk updated this revision to Diff 267344.Fri, May 29, 1:04 PM

Update wording to reflect a change from D80264.

aprantl added inline comments.Fri, May 29, 2:42 PM
llvm/docs/HowToUpdateDebugInfo.rst
3

I like it!

djtodoro added inline comments.Mon, Jun 1, 2:04 AM
llvm/docs/HowToUpdateDebugInfo.rst
22

Having the A Guide for LLVM Pass Authors makes sense to me! Thanks!

233

Makes sense to me, thanks!

This revision was automatically updated to reflect the committed changes.