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

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha marked an inline comment as done.
rochauha added inline comments.


================
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;
+  }
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713