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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1245
+  //
+  // To to get the exact same bytes in re-assembled binary, we disassemble
+  // aamdhsa_next_free_sgpr as the amdgcn.next_free_sgpr assembler symbol and
----------------
rochauha wrote:
> rochauha wrote:
> > scott.linder wrote:
> > > For this and the above case we should have tests to prove this out. I.e. assemble sources to a binary, disassemble and reassemble it, and then compare the two binaries. Ideally we would do this for some edge cases around VGPR/SGPR allocation granularity.
> > > 
> > > There may need to be some fixup between disassembly and reassembly to account for the remaining non-reassembleable bits produced by llvm-objdump, but they should be pretty minor for a trivial kernel, and I would expect you could handle them with just `sed` which seems to be available to LIT tests.
> > Right now we can't really re-assemble in the lit-test. This needs to be tested 'informally' by:
> > 
> > - Manually writing a small test case. Make a copy of it too.  
> > - Assembling it into the binary : Binary-1.
> > - Disassembling it.
> > - Replace the original kernel descriptor with the disassembled kernel descriptor in the copy.
> > - Assemble the copy : Binary-2.
> > - Compare Binary-1 and Binary-2.
> Went this route to check whether re-assembled binaries match or not. Turns out that both binaries match, in size (overall size as well as size of sections)  and also in terms of all the disassembled content. But a `diff object1 object2` says that binary files differ.
I'm not sure I follow what you are describing; my thought was to start with just an asm source file containing only the kernel descriptor directive in the default section, and compare the output of the following (with, e.g. diff, as you mention):

* Assemble it to an object file with llvm-mc
* Assemble it to an object file with llvm-mc | disassemble the kernel descriptor symbol | trim any human-readable prologue |  assemble it to an object file with llvm-mc

As a trivial example, diff doesn't find any difference for the following example:

```
$ printf '.amdhsa_kernel my_kernel\n.amdhsa_next_free_vgpr 0\n.amdhsa_next_free_sgpr 0\n.end_amdhsa_kernel' >a.s
$ release/bin/llvm-mc --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj a.s >a.o
$ diff a.o \
       <(release/bin/llvm-objdump --triple=amdgcn-amd-amdhsa --mcpu=gfx908 --disassemble-symbols=my_kernel.kd a.o \
                 | tail -n +8 \
                 | release/bin/llvm-mc --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj)
```

I don't think you need to use `FileCheck` for these tests at all, you can just rely on ending the RUN pipeline with `diff`, which seems to be supported by lit. You can then just copy-paste the test and edit fields in the input to validate edge cases for things like the SGPR/VGPR allocation directives.

I think more comprehensive testing, including for other sections and executables/DSOs, would be good eventually but for now we should at least have some tests that explicitly confirm the KD disassembly round-trips.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1345
+  return MCDisassembler::Success;
+} // decodeCOMPUTE_PGM_RSRC1()
+
----------------
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.


================
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";
----------------
Can you expand this comment a little and move it to a Doxygen comment for the function?


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1624
+  }
+} // decodeKernelDescriptorDirective()
+
----------------
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]
```


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1630
+  // CP microcode requires the kernel descriptor to be 64 aligned.
+  if (Bytes.size() != 64 || KdAddress % 64 != 0)
+    return MCDisassembler::Fail;
----------------
rochauha wrote:
> scott.linder wrote:
> > The `!= 0` here is redundant.
> I know, but I thought that it is more readable this way. 
Fair enough, in a type-safe language it would be required anyway, so it seems reasonable.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1665
+    Size = 256;
+    return MCDisassembler::SoftFail;
+  }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713