Page MenuHomePhabricator

[AMDGPU] Update AMDGPUUsage with DWARF proposal
ClosedPublic

Authored by t-tye on Nov 20 2019, 6:25 PM.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add DW_OP_LLVM_call_frame_entry_reg and fix typos

  • Add DW_OP_LLVM_call_frame_entry_reg needed for CFI unwinding of VGPRs that are spilled using the EXEC mask on entry.
  • Remove mention of DW_OP_* that are no longer part of the proposal.
  • Correct some names and other minor typos.
scott.linder accepted this revision.Feb 19 2020, 12:26 PM

I think this is at a stage where it is OK to commit it to AMDGPUUsage as a draft proposal, to let us iterate on it before actually proposing it to DWARF proper. It is harder to start a discussion about it without a public document describing it, and it seems easier to refer to it in AMDGPUUsage than to refer to a Phabricator review.

This revision is now accepted and ready to land.Feb 19 2020, 12:26 PM
This revision was automatically updated to reflect the committed changes.

Fair enough. The DWARF committee has a different set of write-up requirements. Let me, aprantl, probinson, or others know if you have issues.

t-tye reopened this revision.Apr 13 2020, 2:57 PM

Reopening to update proposal so more suitable to submit to DWARF site.

This revision is now accepted and ready to land.Apr 13 2020, 2:57 PM
t-tye updated this revision to Diff 257150.Apr 13 2020, 4:43 PM

[AMDGPU] Update DWARF proposal

  • Unify the sections on DWARF expression and location lists.
  • Allow a location description to have one or more single location descriptions.
  • Define context of DWARF expression that includes an initial stack. Allow initial stack to be used when evaluating location list expression with overlapping PC ranges.
  • Reorganize the DWARF proposal in AMDGPUUsage so suitable for submission to the DWARF site.
  • Replace CFI instruction DW_CFA_LLVM_def_cfa_aspace with DW_CFA_def_aspace_cfa and DW_CFA_def_aspace_cfa_sf. This is to avoid the problem that DW_CFA_def_cfa and DW_CFA_def_cfa_sf cannot use a register that is not the size of an address in the CFA address space.
  • Clarify DWARF address class and DWARF address space. Define language values for DWARF address classes and specify how they are used by some common source languages.
  • Define rules for accessing registers and derefencing memory when the type size and register size or byte size operand do not match.
  • Numerous cleanups for consistency.
echristo accepted this revision.Apr 14 2020, 11:54 AM

Enthusiastic LGTM. :)

I would love to see this formally written up and passed along to the committee. Feel free to talk to me, Paul, Adrian, or a few others if you need or want any help.

t-tye added a comment.Apr 14 2020, 2:28 PM

Enthusiastic LGTM. :)

I would love to see this formally written up and passed along to the committee. Feel free to talk to me, Paul, Adrian, or a few others if you need or want any help.

Hi @echristo, Yes that is what I am currently doing. This update is to bring the proposal in line with a form I hope is ready for submission by moving the AMDGPU specific parts out of the proposal into their own section, reordering the sections to match the current DWARF 5 spec order, and adding DWARF 5 section numbers for where the changes apply. There is one more review to move the whole section into a separate file, and add some references. Then I plan to submit to the DWARF issue queue. Some committee members have already reviewed earlier revisions and their feedback has been incorporated. Please let me know if there is anything else you think needs to be done prior to submission.

No, I'm perfectly happy for now.

Thanks for checking!

This revision was automatically updated to reflect the committed changes.
t-tye reopened this revision.Apr 14 2020, 7:36 PM

Reopening to update proposal to move it to a separate file.

This revision is now accepted and ready to land.Apr 14 2020, 7:36 PM
t-tye updated this revision to Diff 257841.Apr 15 2020, 2:07 PM

[AMDGPU] Move DWARF proposal to separate file

  • Move DWARF proposal for heterogeneous debugging to a separate file.
  • Add references.

Differential Revision: https://reviews.llvm.org/D70523

scott.linder accepted this revision.Apr 15 2020, 2:16 PM
This revision was automatically updated to reflect the committed changes.
t-tye reopened this revision.Apr 27 2020, 7:46 PM

Reopen for additional review feedback.

This revision is now accepted and ready to land.Apr 27 2020, 7:46 PM
t-tye updated this revision to Diff 260533.Apr 27 2020, 7:48 PM

[AMDGPU] DWARF proposal review feedback

  • Rename DW_OP_LLVM_offset_constu to DW_OP_LLVM_offset_uconst to matches DW_OP_plus_uconst.
  • Correct DW_OP_LLVM_call_ref to be DW_OP_call_ref.
  • Move proposed changes to a separate section to clarify that the introduction section is not part of the changes.
  • Fix formatting typos and add missing reference.
  • Clarify why DW_OP_LLVM_offset et al do not wrap on overflow.
  • Correct syntax of augmentation string.
t-tye updated this revision to Diff 260537.Apr 27 2020, 8:37 PM

[AMDGPU] DWARF proposal review feedback

  • Minor typo corrections.

Feel free to commit this as you're happy. Probably should have follow on diffs for changes rather than updating this? Up to you, just what's normal :)

t-tye added a comment.Apr 27 2020, 9:58 PM

Thanks @echristo. I had been keeping all the updates on the same review as had provided the link to a number of non-LLVM reviewers so they can see the diffs more easily. @scott.linder had also sent out the link to the review in the RFC. Hope that seems reasonable.

This revision was automatically updated to reflect the committed changes.
scott.linder reopened this revision.Apr 29 2020, 2:15 PM
This revision is now accepted and ready to land.Apr 29 2020, 2:15 PM
scott.linder added a project: debug-info.

Update the tentative encodings to avoid a conflict with a GNU extension.

t-tye accepted this revision.Apr 29 2020, 5:44 PM

LGTM

This was already committed, right? If that is the case, I think we need new Diff representing the change, rather than updating the old one. I think you can append the diff on top of the D76877 stack of patches.

This was already committed, right? If that is the case, I think we need new Diff representing the change, rather than updating the old one. I think you can append the diff on top of the D76877 stack of patches.

We have just continued to update this differential for all the changes to the proposal document. We wanted to make it easy for people to track the development by just looking through the diffs here.

The latest diff hasn't been committed, so I will commit that and close this. For future diffs in the document I guess we could just create a new differential and "stack" it on top of this one?

This revision was automatically updated to reflect the committed changes.
t-tye reopened this revision.May 22 2020, 7:15 PM

Adding minor updates as part of this review instead of creating a new review as the RFC email references this review and seems best to allow all comments within a single review.

This revision is now accepted and ready to land.May 22 2020, 7:15 PM
t-tye updated this revision to Diff 265831.May 22 2020, 7:20 PM

[AMDGPU] DWARF For Heterogeneous Debugging

  • Change title to "DWARF For Heterogeneous Debugging".
  • Add "Examples" section that references the AMDGPUUsage DWARF section.
  • Make the "References" section a top level section.

Differential Revision: https://reviews.llvm.org/D70523

This revision was automatically updated to reflect the committed changes.
t-tye reopened this revision.May 28 2020, 5:19 PM

Add introduction section.

This revision is now accepted and ready to land.May 28 2020, 5:19 PM
t-tye updated this revision to Diff 267095.May 28 2020, 5:21 PM

[AMDGPU] DWARF Proposal For Heterogeneous Debugging

  • Add introduction to DWARF Proposal For Heterogeneous Debugging.

Differential Revision: https://reviews.llvm.org/D70523

This revision was automatically updated to reflect the committed changes.
t-tye reopened this revision.Jul 29 2020, 3:46 PM

Need to add clarification of how context is used in the evaluation of DWARF expressions and how evaluation relates to CFI information.

This revision is now accepted and ready to land.Jul 29 2020, 3:46 PM
t-tye updated this revision to Diff 281776.Jul 29 2020, 6:02 PM

[AMDGPU] DWARF proposal changes for expression context

  • Clarify what context is used in DWARF expression evaluation.
  • Define location descriptions to fully resolve the context and so include the context in their result.
  • As a consequence of location descriptions being fully resoved, change address spaces so only a swizzled and unswizzled private address space is defined. The lane is now part of the location description context.
  • Clarify how call frame information is used to fully resolve expressions that specify registers.
scott.linder accepted this revision.Jul 29 2020, 6:53 PM
This revision was landed with ongoing or failed builds.Jul 29 2020, 6:59 PM
This revision was automatically updated to reflect the committed changes.
t-tye reopened this revision.Jul 29 2020, 9:34 PM

Clarify these are extension to DWARF 5 and not as yet a proposal to DWARF.

This revision is now accepted and ready to land.Jul 29 2020, 9:34 PM
t-tye updated this revision to Diff 281801.Jul 29 2020, 10:04 PM

Clarify that these are extensions to DWARF 5 and not as yet a
proposal.

scott.linder accepted this revision.Jul 29 2020, 10:05 PM
This revision was landed with ongoing or failed builds.Jul 29 2020, 10:08 PM
This revision was automatically updated to reflect the committed changes.