Page MenuHomePhabricator

[NFC][AMDGPU] Refactor AMDGPUDisassembler
ClosedPublic

Authored by scott.linder on Jul 19 2022, 11:32 AM.

Details

Summary

Clean up ahead of a patch to fix bugs in the AMDGPUDisassembler.

Use lit.local.cfg substitutions and more idiomatic use of split-file to
simplify and extend existing kernel-descriptor disassembly tests.

Add a comment to AMDHSAKernelDescriptor.h, as at least one small set
towards keeping all kernel-descriptor sensitive code in sync.

Diff Detail

Event Timeline

scott.linder created this revision.Jul 19 2022, 11:32 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
scott.linder requested review of this revision.Jul 19 2022, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 11:32 AM
arsenm added inline comments.Jul 19 2022, 11:42 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1926

What are these comments?

scott.linder added inline comments.Jul 19 2022, 11:43 AM
llvm/test/tools/llvm-objdump/ELF/AMDGPU/lit.local.cfg
5

I'm not certain I struck the best balance here between "hiding mostly irrelevant details" and "obfuscating the RUN: lines of the KD tests", but I found it hard to tell at a glance what each test case was doing without at least some of the factoring I ended up with here (and the RUN lines were getting long, even with continuations across lines)

Suggestions on this are welcome!

scott.linder added inline comments.Jul 19 2022, 11:45 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1926

They suppress clang-tidy diagnostics, with the idea here being that it is worth using the COMPUTE_PGM_RSRC3 identifier from the KD specification directly in the method name, even though it breaks the normal LLVM style rules

arsenm accepted this revision.Fri, Sep 16, 9:41 AM
arsenm added inline comments.
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-vgpr.s
3

Do you really need to rm first? I don't remember seeing any other test do that

This revision is now accepted and ready to land.Fri, Sep 16, 9:41 AM
kzhuravl accepted this revision.Mon, Sep 19, 7:56 AM

LGTM, thanks!

scott.linder added inline comments.Tue, Sep 20, 1:35 PM
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-vgpr.s
3

I'm also curious where this originates, as it seems around 40% of the uses of split-file are preceded by rm -rf:

$ rg --count-matches split-file | awk -F: '{ sum+=$2 } END { print sum }'
519
$ rg --count-matches 'rm -rf.*split-file' | awk -F: '{ sum+=$2 } END { print sum }'
202

I will remove the rm from the version I land, but it would be nice to understand why it is sometimes present.

This revision was landed with ongoing or failed builds.Tue, Sep 20, 1:38 PM
This revision was automatically updated to reflect the committed changes.
cmtice added a subscriber: cmtice.Tue, Sep 20, 9:45 PM

This commit appears to be causing our bootstrap builds to fail, while running these tests. The error we get for kd-vgpr.s is
[ 5] ;--- 1.s
[ 6] ; RUN: %assemble -mcpu=gfx908 <1.s >1.o [FAIL]
%assemble -mcpu=gfx908 <1.s >1.o
bash: line 0: fg: no job control
Command failed: exit status 1

The error we get for kd-sgpr.s is
[ 6] ;; Only set next_free_sgpr.
[ 7] ; RUN: %assemble -mcpu=gfx908 <1.s >1.o [FAIL]
%assemble -mcpu=gfx908 <1.s >1.o
bash: line 0: fg: no job control
Command failed: exit status 1

Could you revert this commit until these issues are fixed?