This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add gfx10 assembler directive to specify shared VGPR count
ClosedPublic

Authored by lamb-j on Jul 6 2021, 12:30 PM.

Diff Detail

Event Timeline

kzhuravl created this revision.Jul 6 2021, 12:30 PM
kzhuravl requested review of this revision.Jul 6 2021, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 12:30 PM
Herald added a subscriber: wdng. · View Herald Transcript
t-tye added inline comments.Jul 6 2021, 12:42 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4794

How does the value relate to the total VGPR count? Are these in addition, or this just indicates how many of the VGPRs are shared? If the later, is there an error for when this value exceeds the requested VGPR count?

How does new_vgpr work? Is it agnostic to which VGPRs are private and shared? Seems it probably is so that present no issues with adding shared VGPRs but wanted to check.

Also needs disassembler change.

llvm/docs/AMDGPUUsage.rst
12198

Should probably specify it is GFX10 only.

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
437

Only print it if IVersion.Major >= 10.

kzhuravl marked an inline comment as done.Jul 7 2021, 8:03 AM

Also needs disassembler change.

Disassembler support is already here, because the asm printing is done AMDGPUTargetAsmStreamer.

llvm/docs/AMDGPUUsage.rst
12198

It already says "GFX10" in one of the columns.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4794

Will post a follow up patch shortly.

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
437

It is already the case. See line 427 in this patch set.

rampitec accepted this revision.Jul 7 2021, 9:29 AM
This revision is now accepted and ready to land.Jul 7 2021, 9:29 AM
lamb-j added a subscriber: lamb-j.Jan 5 2022, 5:05 PM
lamb-j commandeered this revision.Jan 7 2022, 1:32 PM
lamb-j added a reviewer: kzhuravl.
lamb-j updated this revision to Diff 398226.Jan 7 2022, 1:33 PM
lamb-j marked an inline comment as done.

Add check for shared_vgpr_count exceeding next_free_vgpr

lamb-j added inline comments.Jan 7 2022, 1:39 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4794

I've added a check/error for when shared_vgpr_count exceeds next_free_vgpr. I'm still looking into "new_vgpr" and if any changes need to be made related to that.

lamb-j updated this revision to Diff 403014.Jan 25 2022, 12:52 PM

Adding checks for wavefront size and multiple of 8

lamb-j updated this revision to Diff 404780.Jan 31 2022, 6:56 PM

Updating description to match hardware description, adding appropriate checks

lamb-j retitled this revision from AMDGPU: Add gfx10 assembler directive to specify shared VGPR count to [AMDGPU] Add gfx10 assembler directive to specify shared VGPR count.Jan 31 2022, 6:57 PM
lamb-j updated this revision to Diff 404781.Jan 31 2022, 6:59 PM

Removing extra newline

lamb-j marked an inline comment as done.Jan 31 2022, 7:01 PM
arsenm added inline comments.Jan 31 2022, 7:01 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4880

Braces

4883

Leftover debug printing

4884

Braces, spaces around *

4887

I don't think this needs the newline

lamb-j updated this revision to Diff 404782.Jan 31 2022, 7:06 PM

Removing accidental print statement and fixing formatting

lamb-j marked 4 inline comments as done.Jan 31 2022, 7:07 PM
lamb-j added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4880

Do we want braces here because the TokError() string is split over two lines?

lamb-j requested review of this revision.Feb 1 2022, 9:26 AM
t-tye added inline comments.Feb 7 2022, 2:05 PM
llvm/docs/AMDGPUUsage.rst
4285–4289

Suggest reword to:

Number of shared VGPR blocks for wavefront size 64 when executing in subvector mode. For wavefront size 64 the value is 0-15, representing 0-120 VGPRs (granularity of 8), such that (compute_pgm_rsrc1.vgprs +1)*4 + shared_vgpr_count*8 does not exceed 256. For wavefront size 32 must be 0.
lamb-j updated this revision to Diff 406971.Feb 8 2022, 2:16 PM

Clarifying description

lamb-j updated this revision to Diff 406973.Feb 8 2022, 2:17 PM

Adding revision info to commit message

lamb-j marked an inline comment as done.Feb 8 2022, 2:18 PM
lamb-j updated this revision to Diff 409383.Feb 16 2022, 1:18 PM

Address clang-format comment

lamb-j marked an inline comment as done.Feb 17 2022, 12:04 PM
lamb-j updated this revision to Diff 410601.Feb 22 2022, 11:51 AM

Addressing clang-format comments

kzhuravl accepted this revision.Mar 7 2022, 1:43 PM

LGTM, thanks for taking this over!

This revision is now accepted and ready to land.Mar 7 2022, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 1:43 PM
This revision was landed with ongoing or failed builds.Mar 7 2022, 2:35 PM
This revision was automatically updated to reflect the committed changes.