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

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1252
+  }
+  return None;
+}
----------------
madhur13490 wrote:
> You can return std::nullopt as return type here is std::optional.
llvm::Optional is used here instead of 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 {
----------------
madhur13490 wrote:
> 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.
I was under the impression that having the current order of functions helps to 'incrementally zoom' into the details while reading the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713