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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1233
+
+  // amd_kernel_code_t for Code Object V2
+  if (Symbol.Type == ELF::STT_AMDGPU_HSA_KERNEL) {
----------------
Nit, here and throughout: missing full stop at end of many comments.


================
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);
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1287
+  StringRef ReservedBytes;
+  const std::string Indent = "\t";
+
----------------
Use `StringRef` here.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1289
+
+  DataExtractor DE(Bytes, /*IsLittleEndian =*/true, /*AddressSize =*/64);
+  Error *Err = nullptr;
----------------
clang-format plays better with these sort of comments as below:
```
DataExtractor DE(Bytes, /*IsLittleEndian=*/true, /*AddressSize=*/64);
```

Also, you probably want an address size of 8, not 64 (address size is in bytes).


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1290
+  DataExtractor DE(Bytes, /*IsLittleEndian =*/true, /*AddressSize =*/64);
+  Error *Err = nullptr;
+
----------------
I think it's better to do:
```
Error Err = Error::success();
...
DE.getU32(&CurrentIndex, &Err);
```

and then check the error. The DataExtractor methods handle the `Err` so that it doesn't matter that it's not been checked yet. You'd have to handle the error yourself though. See also my comment about `Cursor`.

If you really think there's no need for the Error, there's no need to specify it at all - in all the `DataExtractor` functions the `Error` parameter is an optional parameter, with nullptr as the default value.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1303
+
+  case 4: // 0 + 4
+    FourByteBuffer = DE.getU32(&CurrentIndex, Err);
----------------
I don't understand what this comment is trying to tell me, especially since the `Size` computation below is `4 + 4`...


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1336
+    }
+    for (int i = 0; i < 20; ++i) {
+      if (ReservedBytes[i] != 0) {
----------------
i -> I


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1464
+  // Decode as directives that handle COMPUTE_PGM_RSRC1
+  const std::string Indent = "\t";
+
----------------
`StringRef`


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1466
+
+  // We can not accurately backward compute #VGPRs used from
+  // GRANULATED_WORKITEM_VGPR_COUNT. So we 'decode' as Code Object V3 predefined
----------------
I think "cannot" is better than "can not".


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1590
+  // Decode as directives that handle COMPUTE_PGM_RSRC2
+  const std::string Indent = "\t";
+
----------------
`StringRef`


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h:81
+                                  uint64_t &Size,
+                                  std::stringstream &KdStream) const;
+
----------------
Use `raw_string_ostream` rather than `std::stringstream`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713