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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D80713#2108650 <https://reviews.llvm.org/D80713#2108650>, @rochauha wrote:

> In D80713#2106033 <https://reviews.llvm.org/D80713#2106033>, @jhenderson wrote:
>
> > No idea about the functional logic of this code, but I do wonder whether you'd be better off dividing it into smaller patches for testability.
>
>
> This patch is for entirely disassembling the kernel descriptor symbol. It happens to be so that many directives affect the kernel descriptor. I will also be adding a new test in this patch.


The problem is that you're trying to do it all at once. There's no need to implement the full disassembly at once. You can do it in a series of patches, one after the other that build towards the primary goal. For example, there are many `if (<X is some value>)` type cases, which you could omit - just assume those are all false, in earlier versions, and add them in (with corresponding testing) in a later patch. Similarly, you could assume that all results of `x & y` return some specific value, e.g. 0, and just print that for now. Yes, that means you won't support everything from the point at which this patch lands, but it will make each individual patch easier to reason with. This fits much better with LLVM's preferred approach - please see https://llvm.org/docs/DeveloperPolicy.html#incremental-development.



================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1265-1268
+  size_t CurrentIndex = 0;
+  while (CurrentIndex < Bytes.size()) {
+    MCDisassembler::DecodeStatus Status =
+        decodeKernelDescriptorDirective(CurrentIndex, Bytes, Size, KdStream);
----------------
jhenderson wrote:
> Have you considered using the `DataExtractor::Cursor` approach (see for example the DWARFDebugLine code)? This will make the offset tracking and error handling cleaner I think.
I don't see a response to this suggestion.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1326
+
+  // next two bits are reserved amd must be 0.
+  if ((FourByteBuffer & amdhsa::COMPUTE_PGM_RSRC1_RESERVED0) >>
----------------
next -> Next


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1472
+
+  DataExtractor DE(Bytes, /*IsLittleEndian =*/true, /*AddressSize =*/8);
+
----------------
`/*IsLittleEndian =*/true` -> `/*IsLittleEndian=*/true`

(and same for `AddressSize`)

as already asked for once...


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1662
+
+  // amd_kernel_code_t for Code Object V2
+  if (Symbol.Type == ELF::STT_AMDGPU_HSA_KERNEL) {
----------------
Nit: missing full stop.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1664-1665
+  if (Symbol.Type == ELF::STT_AMDGPU_HSA_KERNEL) {
+    // Right now this condition will always evaluate to false due to above
+    // mentioned issue.
+
----------------
Put this comment above the `if` it is referring to, like the exisitng bit of comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713