This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][1/*] Add initial docs for Assignment Tracking
ClosedPublic

Authored by Orlando on Aug 19 2022, 6:14 AM.

Details

Reviewers
jryans
Summary

The Assignment Tracking debug-info feature is outlined in this RFC. This first series of patches adds documentation, the changes necessary to start emitting and using the new metadata, and updates clang with an option to enable the feature. Working with the new metadata in the middle and back end will come later. There are still a few rough edges but I'm putting these patches up now hoping to get feedback on the design and implementation from the upstream community.


Add a brief document outlining the intent and design.

Diff Detail

Event Timeline

Orlando created this revision.Aug 19 2022, 6:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2022, 6:14 AM
Orlando requested review of this revision.Aug 19 2022, 6:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2022, 6:14 AM
jmorse added a subscriber: jmorse.Aug 30 2022, 7:02 AM

Not a major nit, but something occasionally encountered: IMO the word "debug" needs to occur somewhere in the opening paragraph, or even title. The compiler as a whole covers a pretty wide range of topics, and someone stumbling across the page at random might not immediately know that it's about debug-info.

Sounds good; not hitting "accept" just yet until we've waved these patches in the usual directions.

llvm/docs/AssignmentTracking.md
6–8

IMO this could be interpreted quite widely -- I think the point is to indicate that the technique isn't perfectly 100% sound, in which case perhaps we could say "in rare and complicated circumstances indirect assignments might be optimized away without being tracked, but otherwise we make our best effort to track all variable locations". The word "reasonable" does, alas, mean different things to different people.

13

"what we have now" will immediately go stale, how about "to using dbg.declare and dbg.value"

50–58

Probably best to ditch this IMO; or put the 2nd signature first and say "the formal signature in LLVM-IR is <metadata*5>...". That avoids the reader thinking they're reading redundant things.

76–78

I'd suggest fitting the word "node" in here somewhere just to be explicit that this is part of the existing DI metadata hierarchy, rather than some other meta-metadata.

112

asssign -> assign

128

Dunno how the elements below render, but presumably they want capitalization ("Cloning", "Moving" etc).

Also IMO, it's worth moving moving / deleting debug instructions to their own list, so that there's a collection of "things to do to Real instruction" and "things to do to debug instructions", which decomposes a little nicer IMHO, YMMV.

191

I think we've discussed existing limitations elsewhere -- does it make sense to put a few in here as a very high level "TODO" list, as it were? No point if it's a substantial amount of work, but if there's already a list, that could help future readers.

jryans added a subscriber: jryans.Aug 31 2022, 1:38 PM

Overall, this looks great! Thanks for writing it up. 😄

Apologies for all of my style nits that follow... 😅

llvm/docs/AssignmentTracking.md
2

Please also link to this new page from some list of pages to make it easier to find, such as UserGuides.rst (most likely both in the page list and in the TOC tree). Search for InstrRefDebugInfo for a recent example of doing this for another Markdown-formatted doc.

5

Nit: When mentioning LLVM (the system, as opposed to a class or namespace) in text, most docs are fairly consistent in spelling it as "LLVM", so it would be good to follow that style here as well. (Apologies if this seems ultra-nitty...! 😅)

18

Does the guide for pass authors need any additions or tweaks to cover this new intrinsic? Since this is currently an experimental feature, perhaps it doesn't make sense merge all your pass-author guidance on Assignment Tracking into that existing doc, but at the very least, a few well-placed links to this doc seem like a good idea to me.

25

Nit: As with "LLVM", use the name "Clang" in text.

27
31
74
80

"use" is not IR syntax, so perhaps normal quotes are better to parallel "refers to" on the next line.

174
178
183
llvm/docs/SourceLevelDebugging.rst
265

AFAIK, the *.rst format needs two ticks for this... (Isn't writing in two document formats fun...?! 🤪)

266

Use ticks around "DIAssignID" here:

``DIAssignID``
270

You should be able to use the following to link the new page (even though it's in Markdown):

See :doc:`AssignmentTracking` for more info.

Also, you are currently pointing to the InstrRefDebugInfo page, but probably you meant your new AssignmentTracking instead...?

(Phabricator wouldn't let me suggest an edit here for some reason... 🤔)

jryans added inline comments.Aug 31 2022, 2:05 PM
llvm/docs/SourceLevelDebugging.rst
259

I think it would be nice to paste the C++ "signature in practice" over here as well... (The others in this list could use it too, but that's for another patch...)

I don't really think any one is going to make much sense out of just this list of 6 metadatas... (I've never quite followed why this section uses that style...)

Orlando updated this revision to Diff 458136.Sep 6 2022, 3:53 AM
Orlando marked 22 inline comments as done.

+ Address review comments.

Thank you both for the reviews!

llvm/docs/AssignmentTracking.md
2

Done - thanks for the example. You may notice that the InstrRefDebugInfo entry is conspicuously missing from the diff base, because it's a little old. I'll rebase everything as I start landing thing - I've put the AssignmentTracking references in UserGuides.rst next to where InstrRefDebugInfo comes in latest main.

5

Apologies if this seems ultra-nitty...!

I appreciate the detailed review :-)

18

Good point, I've added a link from HowToUpdateDebugInfo.rst.

76–78

like so?

jryans accepted this revision.Sep 6 2022, 4:26 AM

Looks good overall, thanks! 😄

llvm/docs/AssignmentTracking.md
8
167
This revision is now accepted and ready to land.Sep 6 2022, 4:26 AM
Orlando updated this revision to Diff 458998.Sep 9 2022, 2:44 AM
Orlando marked 2 inline comments as done.

+ Address comments
+ Add TODO list item regarding Poison / Undef values

Orlando updated this revision to Diff 459001.Sep 9 2022, 2:51 AM

+ Minor formatting change. Sorry for the noise.

Orlando closed this revision.Nov 2 2022, 6:51 AM

I accidentally left off the review link in the patch - landed this in 33c7ae55e729069be754f56c4d4606cdeddd377b. Thanks again for the reviews.