This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][NFC] Add instr-ref documentation, migration guide
ClosedPublic

Authored by jryans on Nov 10 2021, 10:43 AM.

Details

Summary

This used to be D102158, but all the code it describes got re-written, so I figured I'd take another shot at documenting the new instruction referencing variable locations, this time from a higher level. Happily there's no longer any need to describe LiveDebugValues in any detail seeing how it's all SSA-based now.

Probably the most important part is the explanation of what targets need to do to support instruction referencing. The list is small, mostly because there's nothing especially complicated that targets need to do: just instrument their target-specific optimisations and implement the stack spill/restore recognition target hooks.

This is a small amount of text (which is a virtue), I'm extremely happy to expand on anything.

Diff Detail

Event Timeline

jmorse requested review of this revision.Nov 10 2021, 10:43 AM
jmorse created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 10:44 AM

Nice! Should we link to this document from other documents?

llvm/docs/InstrRefDebugInfo.md
3

Can we concretize "final stages" by explicitly calling out MIR or AsmPrinter?

4

Could you add a "who should read this document" sentence?

Thanks!

llvm/docs/InstrRefDebugInfo.md
83
144

nit: are we missing a = here? (and below)

170

double ;;

jryans added a subscriber: jryans.Apr 28 2022, 9:10 AM

@jmorse It would be great to get this landed, as it's quite a useful reference to have around. 😄 If the main blocker is lack of time, I'd be happy to take over and work through the review comments so this can move forward.

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 9:10 AM

@jmorse It would be great to get this landed, as it's quite a useful reference to have around. 😄 If the main blocker is lack of time, I'd be happy to take over and work through the review comments so this can move forward.

Ah. Uh. Not lack of time really, it's just been a mental hurdle to get my mind back into documentation-writing mode.

If you're interested in driving this forwards it'd be highly appreciated. I'll probably get around to this in another week or two otherwise, I'm periodically reminded elsewhere that I should have landed this ages ago.

jryans added a comment.May 3 2022, 1:55 PM

If you're interested in driving this forwards it'd be highly appreciated. I'll probably get around to this in another week or two otherwise, I'm periodically reminded elsewhere that I should have landed this ages ago.

The next few weeks are a bit busy for me with holidays and conferences, but once that settles down, I should be able to get this polished up if you haven't already done so by then.

jryans commandeered this revision.May 10 2022, 6:34 AM
jryans added a reviewer: jmorse.

I've had a bit of time to work on this, so I'll post my revised version momentarily... I wasn't sure if I should commandeer this revision or start my own, but I figured commandeering this one seemed best to keep the conversation in one place.

jryans updated this revision to Diff 428334.May 10 2022, 6:38 AM

I've made various small improvements in this version:

  • Added incoming links to the new doc
  • Added a doc title
  • Linked to LiveDebugValues in the source-level debugging doc
  • Clarified when this work happens
  • Add notes on who should read this doc
  • Tweaked spelling and style
jryans marked 5 inline comments as done.May 10 2022, 6:39 AM

All those revisions / updates look good to me -- @StephenTozer @Orlando would you be able to give another look-over?

Also, many thanks for driving this. After much introspection, I think that every time I came back to this document I tried to rewrite it from scratch, which is kind of a blocker against forward progress :(.

Orlando accepted this revision.May 17 2022, 9:03 AM

LGTM with some nits addressed, thanks! I've hit accepted but please leave this up for a day or so in case anyone else has any further comments.

llvm/docs/InstrRefDebugInfo.md
13

nit: variable's

42

nit: instruction's

91

nit: is separated -> are separated?

96

nit: LLVM's

154–155

nit: I found the first sentence of this paragraph a little difficult to parse - possible alternative suggested above.

161

nit: Removing this comma improves readability IMO.

168

nit: should register defines be register definitions? I'm not sure if this is just terminology I'm not familiar with.

This revision is now accepted and ready to land.May 17 2022, 9:03 AM

Also, many thanks for driving this. After much introspection, I think that every time I came back to this document I tried to rewrite it from scratch, which is kind of a blocker against forward progress :(.

No worries, happy to help. 😄 I also considered making some bigger changes, but in the end I felt it was better to first land what's here so it's more widely available, and then we can always improve further in future changes.

jryans marked 7 inline comments as done.May 18 2022, 7:27 AM
jryans added inline comments.
llvm/docs/InstrRefDebugInfo.md
168

Ah yes, I believe so, "definition" is the more common term in LLVM and also the more typical way I heard it phrased generally, so I've changed this to match your suggestion. There a few spots that do call them "defines", but my guess it's that's mainly because of the enum value RegState::Define perhaps leading people to call them "defines".

jryans updated this revision to Diff 430374.May 18 2022, 7:27 AM
jryans marked an inline comment as done.

Addressed review feedback.

This revision was landed with ongoing or failed builds.May 20 2022, 6:14 AM
This revision was automatically updated to reflect the committed changes.