[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:1345
+  return MCDisassembler::Success;
+} // decodeCOMPUTE_PGM_RSRC1()
+
----------------
scott.linder wrote:
> rochauha wrote:
> > scott.linder wrote:
> > > I don't know the general conventions here, but I don't think I have seen a comment for the end of a function elsewhere in LLVM. I do know that it is required for namespaces, so maybe it is permitted for long functions?
> > I'm not sure. I added those comments because these functions were getting quite long.
> I would lean towards omitting these, especially with the functions becoming shorter. For example, `decodeCOMPUTE_PGM_RSRC2()` is now <50 lines long at the entire definition now fits on one screen for me. It seems like there are other examples of this in the codebase, though, so I'm OK with it for the longer functions.
Done.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1349
+    uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
+  // Decode as directives that handle COMPUTE_PGM_RSRC2.
+  StringRef Indent = "\t";
----------------
scott.linder wrote:
> Can you expand this comment a little and move it to a Doxygen comment for the function?
Done.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1624
+  }
+} // decodeKernelDescriptorDirective()
+
----------------
scott.linder wrote:
> Need to handle the "default" case here:
> 
> ```
> llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1510:1: warning: control may reach end of non-void function [-Wreturn-type]
> ```
Done.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1665
+    Size = 256;
+    return MCDisassembler::SoftFail;
+  }
----------------
scott.linder wrote:
> rochauha wrote:
> > scott.linder wrote:
> > > I'm still not sure what we landed on for the semantics of `SoftFail` here?
> > It should be Success / Fail based on what the bytes are for code object v2. But there's nothing we are 'doing' at the moment for v2, I returned SoftFail.
> If `SoftFail` isn't applicable I don't think we should return it, even if it is just because we haven't implemented something yet. It existing doesn't mean it needs to be used, I think it has a very narrow definition that doesn't apply here.
> 
> Maybe just emit a diagnostic and return `Fail` so we get the "decode as .byte" behavior? What exactly happens now with the current patch as-is?
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713