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

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha added a comment.

> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713