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

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1252
+  }
+  return None;
+}
----------------
You can return std::nullopt as return type here is std::optional.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1255
+
+MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeKernelDescriptor(
+    StringRef KdName, ArrayRef<uint8_t> Bytes, uint64_t KdAddress) const {
----------------
Can you please have function in reverse order? E.g."onSymbolStart" calls "decodeKernelDescriptor" so latter should be above former. This way it's readable and and at a predictable location. Ideally, callees should be above callers.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1399-1414
+      // if (Obj->isELF() && Obj->getArch() == Triple::amdgcn) {
+      //   if (Symbols[SI].Type == ELF::STT_AMDGPU_HSA_KERNEL) {
+      //     // skip amd_kernel_code_t at the begining of kernel symbol (256
+      //     bytes) Start += 256;
+      //   }
+      //   if (SI == SE - 1 ||
+      //       Symbols[SI + 1].Type == ELF::STT_AMDGPU_HSA_KERNEL) {
----------------
rochauha wrote:
> kzhuravl wrote:
> > Why is this commented out?
> `if (Symbols[SI].Type == ELF::STT_AMDGPU_HSA_KERNEL)` always evaluates to false. This is because there are two symbols for AMDGPU kernels:
> 
> `<kernel>` => the proper symbol (`STT_AMDGPU_HSA_KERNEL`)
> `<kernel>$local` => the additional symbol of `STT_NOTYPE`
> 
> Both these symbols point to the same location. Now `<kernel>` is skipped early on in this loop because it is at the beginning of the next symbol in the list `<kernel>$local`
> 
> Additionally, this is symbol specific behavior and is being handled in onSymbolStart in the disassembler implementation.
> 
> 
> 
> `if (SI == SE - 1 ||  Symbols[SI + 1].Type == ELF::STT_AMDGPU_HSA_KERNEL)`
> 
> The first part of this condition evaluates to true for the last symbol in a section. Kernel descriptors are usually appear the .rodata section of the binary. Last 6 bytes of the kernel descriptor are reserved and must be zero. Due to this condition, we end up trimming last 4 bytes of the kernel descriptor, making it 'invalid'.
Can we not remove it? Why to keep commented in code base?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713