[PATCH] D80713: [AMDGPU] Support disassembly for AMDGPU kernel descriptors

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha marked 7 inline comments as done.
rochauha added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s:1
+; RUN: llvm-mc < %s -mattr=+code-object-v3 --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj -o %t1
+; RUN: llvm-objdump --triple=amdgcn-amd-amdhsa --mcpu=gfx908 --disassemble-symbols=my_kernel.kd %t1 | tail -n +8 | llvm-mc --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj -o %t2
----------------
jhenderson wrote:
> No need for the `<`. llvm-mc is quite capable of taking inputs on the command-line as positional arguments.
Done.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s:2
+; RUN: llvm-mc < %s -mattr=+code-object-v3 --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj -o %t1
+; RUN: llvm-objdump --triple=amdgcn-amd-amdhsa --mcpu=gfx908 --disassemble-symbols=my_kernel.kd %t1 | tail -n +8 | llvm-mc --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj -o %t2
+; RUN: diff %t1 %t2
----------------
jhenderson wrote:
> This line is too long. Please break it up into individual lines:
> ; RUN: llvm-objdump ... | \
> ; RUN:   tail -n +8 | llvm-mc ...
Done.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s:5-8
+.amdhsa_kernel my_kernel
+.amdhsa_next_free_vgpr 50
+.amdhsa_next_free_sgpr 2
+.end_amdhsa_kernel
----------------
jhenderson wrote:
> This test is also quite small. Does it actually cover every code path?
These values must be always specified by the user. Values of some bytes are computed using the values passed here. Consequently there are some cases where we need to test for getting the exact same bytes in the re-assembled binary, even if the disassembled values slightly deviate from the original values.

Other bytes/bits hold the exact values specified using the assembler directives. Also they take default values if nothing is specified by the user. So I guess we don't need to test that for re-assembly.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s:10
+
+; Right now this test fails for some combinations for the two directives above. For example (50, 23) and (42, 42)
----------------
jhenderson wrote:
> Is this a FIXME/TODO? If so, please add "FIXME" or "TODO".
Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80713/new/

https://reviews.llvm.org/D80713