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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

> > 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.
>
>
>
> > Oh, you could also put the additional behaviour behind a switch in llvm-objdump. That'll mean you won't break all the existing tests too. Once you've finished the development, you could either remove the switch entirely or just change its default (which would allow users to disable the behaviour if they want the less verbose version of the output).
>
> Thanks for the feedback! I'll keep these points in mind :)
>
> Regarding this patch - I think that because it has been reviewed to quite some extent, it doesn't need to be broken into smaller patches now?


Maybe I missed something from some of the other reviewers, but I haven't seen much in the way of commentary on the core functionality of the new code, so I wouldn't say that it has been reivewed to quite some extent. I've personally  only skirted around style aspects. I also don't see any evidence of any testing of the new code.



================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1476
+  // The failed region is from 0 to this new value of Size. We do this because
+  // most directives in the kernel descriptor affect a single or very few bits.
+  switch (Cursor.tell()) {
----------------
Bits or bytes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713