[PATCH] D96906: [AMDGPU] gfx90a support

Thu Feb 18 17:05:02 PST 2021

kzhuravl added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:737
   EF_AMDGPU_MACH_AMDGCN_FIRST = EF_AMDGPU_MACH_AMDGCN_GFX600,
-  EF_AMDGPU_MACH_AMDGCN_LAST = EF_AMDGPU_MACH_AMDGCN_GFX805,
+  EF_AMDGPU_MACH_AMDGCN_LAST = EF_AMDGPU_MACH_AMDGCN_GFX90A,

----------------
tra wrote:
> Nit: This looks odd. GFX90A does not need to be in the middle of the list. It makes it somewhat confusing to tell which ID is really the last. The `_LAST` enum says it's GFX90A, but it's not the last item of the list. There are already out-of-name-order GPUs at the end of the list, so putting 90A at the end would probably be a better choice. At least we'd still have the numeric values in order. Right now the list is ordered neither by the name nor by the value.
> 
> There's also a question of whether something needs to be done about the missing values 0x3c..0x3e. Presumably the `_FIRST`..`_LAST` enums specify the range we'll use to iterate over the GPU IDs. Do we handle the missing values correctly?
> 
> Looks like it's benign at the moment as we're only using it to return amdgcn triple in ELFObjectFile.h. I'd add the placeholder enums for the reserved/unused values within the range.
> 
https://reviews.llvm.org/D97010

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96906