Page MenuHomePhabricator

[AMDGPU] Improve assembler + disassembler handling of kernel descriptors

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


  • 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

diagnostics are recommended not to be capitalized. llvm-objdump sticks with this convention quite well:

MaskRay added inline comments.Thu, Jul 14, 10:22 AM

the cast is redundant.


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)


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

Can this be a plain bool?


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

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.

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