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

Scott Linder via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 6 13:42:14 PDT 2020


scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1395
+  assert(Bytes.size() == 64);
+  DataExtractor DE(Bytes, /*IsLittleEndian=*/true, /*AddressSize=*/8);
+
----------------
rochauha wrote:
> scott.linder wrote:
> > scott.linder wrote:
> > > scott.linder wrote:
> > > > rochauha wrote:
> > > > > I think this is where little endian is being 'hardcoded'. However, since AMDGPU relocatable objects are meant to be little endian, I don't understand why they are big endian on pp64.
> > > > I think this is actually a bug with the encoding code. Like you say, we should be host-endianness agnostic when encoding the kernel descriptor, but it seems we aren't.
> > > > 
> > > > I think I vaguely remember this coming up when we implemented it, but I don't remember why we didn't do this to start. I think it is just an oversight.
> > > I think https://reviews.llvm.org/D88858 should be the fix, need to confirm if big-endian testers will run it automatically.
> > I tried to determine if the pre-checkin builders include any big-endian archs, but gave up and just committed it. I'll keep an eye on the builders and see if it needs to be revered. After that you can proceed with this patch again.
> Thanks!
An update, I missed one test in the initial commit, but I followed up in bf5c1d92d92ef8cee2adbfa17ecca20a8f65dc0e and now the big-endian testers seem to be happy. I think you can try reapplying this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713



More information about the llvm-commits mailing list