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

Ronak Chauhan via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 3 01:03:15 PDT 2020


rochauha added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1472
+  DataExtractor DE(Bytes, /*IsLittleEndian=*/true, /*AddressSize=*/8);
+  // When we fail, we set:
+  //    Size = current cursor position (starting point of the chunk of bytes)
----------------
scott.linder wrote:
> I don't understand the intent with carefully maintaining `Size` for the failure case. Aren't we certain by this point that this //should// be a kernel descriptor, and so the correct thing to do when we fail is to disassemble everything as `.byte` directives (i.e. set `Size` to the max value)? Why would we prefer to return a partial failure and have the disassembler start working on the remaining bytes as if they were instructions?
> 
> That would also shorten the patch and make it obvious that the `Size` tracking is correct.
My initial take was to decode as byte directives to the point of failure indicated by Size. Then going back to the normal flow of disassembling as instructions.

But I get what you want to say in this comment.

Updated the code based on this comment.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1635
+  raw_string_ostream KdStream(Kd);
+  KdStream << ".amdhsa_kernel " << KdName.drop_back(3).str() << "\n";
+
----------------
scott.linder wrote:
> Could you move the call to `.drop_back(3)` out into the calling function, so it appears next to the check for `.endswith(StringRef(".kd"))`?
Done.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h:83
+
+  DecodeStatus decodeCOMPUTE_PGM_RSRC1(uint32_t FourByteBuffer,
+                                       raw_string_ostream &KdStream) const;
----------------
scott.linder wrote:
> Could you either rename this to satisfy the linter, or else explicitly suppress the lint with a comment like:
> 
> ```
> // NOLINTNEXTLINE(readability-identifier-naming)
> ```
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713





More information about the llvm-commits mailing list