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

Mon Aug 3 00:08:01 PDT 2020

jhenderson added a comment.

In D80713#2187653 <https://reviews.llvm.org/D80713#2187653>, @scott.linder wrote:

> In D80713#2186791 <https://reviews.llvm.org/D80713#2186791>, @jhenderson wrote:
>
>> I'm probably not in a position to review the majority of this further. However, I do have big reservations about the testing - there are a high number of possible code paths, but I only see 5 tests, which clearly can't cover all these code paths.
>
> I think some of the difficulty in writing tests for the failure cases is not having a tool to produce them. I think one would have to generate the code object, and then hand-edit it in a hex-editor, copy it into the source tree (i.e. under `Inputs/`) maybe with some accompanying comment about the nature of the original source and the edits made?
>
> It would be nice to at least cover some of the failure cases, even if they are more awkward to tests. There could also be some similar tests for e.g. GFX7,GFX10 and the edge cases around the {S,V}GPR calculation for each.

It seems to me that the format of the data structure is well-understood (otherwise you wouldn't be able to write code to disassemble it). In similar situations, e.g. the DWARF .debug_line parsing, we didn't rely on the built-in .file/.loc directives to generate our line table, and instead wrote it out by hand using .byte/.half/.long/.quad. It's not the prettiest of things of course, but it's better than code paths that never get exercised in tests. You could also use yaml2obj to do a generate the section with a raw hex blob, and/or write an array of bytes in a gtest unit test. The latter situation might be particularly useful because it would allow you to use the same "base" array, and modify individual bytes in individual test cases to check the behaviour for each.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713