Page MenuHomePhabricator

[AMDGPU] Update AMDGPUUsage with DWARF proposal
ClosedPublic

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

Diff Detail

Event Timeline

t-tye created this revision.Nov 20 2019, 6:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 6:25 PM
t-tye retitled this revision from Update AMDGPUUsage with AMDGPU DWARF proposal and other fixes to [AMDGPU] Update AMDGPUUsage with DWARF proposal and other fixes.Nov 20 2019, 6:44 PM
t-tye updated this revision to Diff 230753.EditedNov 23 2019, 12:16 AM

Add rule for DW_OP_stack_value applied to an implicit pointer value to push an implicit pointer location description.

t-tye updated this revision to Diff 230754.EditedNov 23 2019, 12:32 AM

Move note on supporting any integral value to be implicitly converted to a location description.

Can you split this up?

t-tye updated this revision to Diff 233504.Dec 11 2019, 10:48 PM

Split non-DWARF changes into D71392.

Can you split this up?

@echristo I split the non-DWARF changes into D71392.

t-tye retitled this revision from [AMDGPU] Update AMDGPUUsage with DWARF proposal and other fixes to [AMDGPU] Update AMDGPUUsage with DWARF proposal.Dec 11 2019, 10:54 PM
t-tye edited the summary of this revision. (Show Details)

The proposal seems interesting, but needs some more review.

In the meantime, have you sent it to http://dwarfstd.org/Issues.php ?

t-tye updated this revision to Diff 245473.Feb 19 2020, 10:59 AM

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.Mon, Apr 27, 7:46 PM

Reopen for additional review feedback.

This revision is now accepted and ready to land.Mon, Apr 27, 7:46 PM
t-tye updated this revision to Diff 260533.Mon, Apr 27, 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.Mon, Apr 27, 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.Mon, Apr 27, 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.Wed, Apr 29, 2:15 PM
This revision is now accepted and ready to land.Wed, Apr 29, 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.Wed, Apr 29, 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.Fri, May 22, 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.Fri, May 22, 7:15 PM
t-tye updated this revision to Diff 265831.Fri, May 22, 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.