Page MenuHomePhabricator

[AMDGPU] Improve assembler + disassembler handling of kernel descriptors
AcceptedPublic

Authored by scott.linder on Jun 16 2022, 3:02 PM.

Details

Summary
  • Relax the AsmParser to accept .amdhsa_wavefront_size32 0 when the .amdhsa_shared_vgpr_count directive is present.
  • Teach the KD disassembler to respect the setting of KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32 when calculating the value of .amdhsa_next_free_vgpr.
  • Teach the KD disassembler to disassemble COMPUTE_PGM_RSRC3 for gfx90a and gfx10+.
  • Include "pseudo directive" comments for gfx10 fields which are not controlled by any assembler directive.
  • Fix disassembleObject failure diagnostic in llvm-objdump to not hard-code a comment string, and to follow the convention of not capitalizing the first sentence.

Diff Detail

Event Timeline

scott.linder created this revision.Jun 16 2022, 3:02 PM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
scott.linder requested review of this revision.Jun 16 2022, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 3:02 PM
This revision is now accepted and ready to land.Thu, Jul 14, 9:18 AM
MaskRay added inline comments.Thu, Jul 14, 10:20 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1443

diagnostics are recommended not to be capitalized. llvm-objdump sticks with this convention quite well: https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

MaskRay added inline comments.Thu, Jul 14, 10:22 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
2107

the cast is redundant.

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-gfx10.s
4

When there are multiple files, consider:

; RUN: rm -rf %t && split-file %s %t && cd %t

then just use relative filenames below (remove all %t)

7

In several test/tools, the convention is to place | at the end of the previous line and indent the continuation line(s) by 2 spaces.

MaskRay added inline comments.Thu, Jul 14, 10:25 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
100

Can this be a plain bool?

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-zeroed-gfx10.s
4

I'd prefer -s before -d because in the llvm-objdump output, the -s part precedes the -d part.

rochauha added inline comments.Fri, Jul 15, 12:07 PM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
100

I think using Optional is appropriate. The .amdhsa_wavefront_size32 directive and consequently EnableWavefrontSize32 here are specific to GFX10-11. Otherwise that bit is reserved and must be 0 for older hardware.

scott.linder edited the summary of this revision. (Show Details)

Address feedback

Split out parts of the patch into NFC

scott.linder marked 5 inline comments as done.Tue, Jul 19, 11:43 AM
scott.linder added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
100

Right, like Ronak says the nominal type for this parameter to AMDGPU::IsaInfo::getVGPREncodingGranule is Optional<bool>, and using plain bool here would be assuming some details about the implementation in IsaInfo for limited benefit