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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

@kzhuravl, do you have any additional comments to add at all, since you rejected the original patch?



================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1394
+
+  DataExtractor DE(Bytes, /*IsLittleEndian=*/true, /*AddressSize=*/8);
+
----------------
I think for self-documentation purposes, it would be helpful to assert that `Bytes.size() == 64` here. I see that it is verified in the calling function but a) that's not obvious when looking at this function in isolation, and b) in the future, we don't want other places calling this code without that check, so the assert provides a backstop of sorts.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1542-1548
+  while (C && C.tell() < Bytes.size()) {
+    MCDisassembler::DecodeStatus Status =
+        decodeKernelDescriptorDirective(C, Bytes, KdStream);
+
+    if (Status == MCDisassembler::Fail)
+      return MCDisassembler::Fail;
+  }
----------------
rochauha wrote:
> jhenderson wrote:
> > If this loop terminates due to `C` being invalid, you don't want to fall out the bottom and return `Success`, I think. You'd want to check `C` after loop termination and return `Fail`. Alternatively, if you return `Fail` from the `decodeKernelDescriptorDirective` you can do `cantFail(C.takeError());` after the loop.
> > 
> > Ideally, we'd actually report the error from `C` in the event of failure, but currently there's no way of communicating that back up to the caller.
> Thanks for the pointer! I think `cantFail` seems to be all that is needed, because the cursor was holding Error::success in some cases, which needs to be 'checked' before moving further. 
> 
> Since the kernel descriptor is well defined, all cases where failure needs to be handled are handled using `MCDisassembler::Fail`.
Ah, I missed that the input can't be truncated, so `C` can't get into a failure state itself. Thanks! (See also my comment about an assert above).


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s:2
+;; Failure test. We create a malformed kernel descriptor (KD) by manually
+;; setting the bytes. Because in actuality, one can't create a malformed KD
+;; using the assembler directives.
----------------



================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-sgpr.s:22
+;--- 1.s
+;; Only set next_free_sgpr
+.amdhsa_kernel my_kernel_1
----------------
Nit: missing full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713