[PATCH] D38386: AMDGPU: Add ELFOSABI_AMDGPU_PAL

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: include/llvm/Object/ELFObjectFile.h:1067-1076
+    switch (EF.getHeader()->e_ident[ELF::EI_CLASS]) {
+    case ELF::ELFCLASS32:
+      return Triple::UnknownArch;
+    case ELF::ELFCLASS64:
+      switch (EF.getHeader()->e_ident[ELF::EI_OSABI]) {
+      case ELF::ELFOSABI_AMDGPU_HSA:
+      case ELF::ELFOSABI_AMDGPU_PAL:
----------------
Perhaps this should be:


```
if (EF.getHeader()->e_ident[ELF::EI_CLASS] != ELF::ELFCLASS64)
  return Triple::amdgcn;
return Triple::UnknownArch;
```

The amdgcn only supports 64 bit and is independent of the OS.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3521
       // Handle architecture specific OS/ABI values.
-      if (e->e_machine == ELF::EM_AMDGPU &&
-          e->e_ident[ELF::EI_OSABI] == ELF::ELFOSABI_AMDGPU_HSA)
-        W.printHex("OS/ABI", "AMDGPU_HSA", ELF::ELFOSABI_AMDGPU_HSA);
+      if (e->e_machine == ELF::EM_AMDGPU)
+        W.printEnum("OS/ABI", e->e_ident[ELF::EI_OSABI],
----------------
Should this be:


```
if (e->e_machine == ELF::EM_AMDGPU && (e->e_ident[ELF::EI_OSABI] >= ELF::EI_FIRST_ARCH && e->e_ident[ELF::EI_OSABI] <= ELF::EI_LAST_ARCH))
```

Since only this range is the architecture specific, and other values should be handled by the ElfOSABI table. Technically there are some entries in that table that are architecture specific and probably ought to be handled in a similar way so that different architectures can use the same values, but if desired that can be done as another patch.


https://reviews.llvm.org/D38386