[AMDGPU] Add AMDGPU support for llvm-objcopy

[aakanksha555]

Summary

llvm-objcopy does not support AMDGPU currently. It does not get target names where all other tools do but has it's own list. Update the list to support amdgpu.

[jhenderson]

Test case?

[aakanksha555]

Ping.
Would appreciate some feedback on this patch.

[jhenderson]

Sorry, I was away last week and am still catching up since then.

Is there a reason you haven't just added the new amdgpu case to the existing cross-arch-headers.test file? That seems to me like where it should belong.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

305–306

I would suggest that this be moved to the end of the list, purely on the basis that the list has no specific ordering, and newer targets tend to be added at the end.

[aakanksha555]

In D143539#4517371, @jhenderson wrote:

Sorry, I was away last week and am still catching up since then.

Is there a reason you haven't just added the new amdgpu case to the existing cross-arch-headers.test file? That seems to me like where it should belong.

Thanks for the feedback.

I ended up putting it in a separate testcase due to how amdgcn arch is handled - https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Object/ELFObjectFile.h#L1327
I'm specifying the relevant flag EF_AMDGPU_MACH_AMDGCN_GFX900 in this standalone test.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

305–306

I'll move it at the bottom of the list

[aakanksha555]

Moved amdgpu to the bottom of the list

[jhenderson]

In D143539#4519733, @aakanksha555 wrote:

In D143539#4517371, @jhenderson wrote:

Sorry, I was away last week and am still catching up since then.

Is there a reason you haven't just added the new amdgpu case to the existing cross-arch-headers.test file? That seems to me like where it should belong.

Thanks for the feedback.

I ended up putting it in a separate testcase due to how amdgcn arch is handled - https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Object/ELFObjectFile.h#L1327
I'm specifying the relevant flag EF_AMDGPU_MACH_AMDGCN_GFX900 in this standalone test.

There's no need for an entirely new test file just to add a flag. You can modify the existing YAML in cross-arch-headers.test to have a configurable Flags field. Simply add a field Flags: [[FLAGS=<none>]] to the YAML header and then invoke yaml2obj again with an additional option: -DFLAGS=[EF_AMDGPU_MACH_AMDGCN_GFX900]. It may also be worth checking whether the flag is present in the output. The =<none> part means the field will be treated as omitted by default, if the -D option isn't specified.

That all being said, the requirement for the flag is troubling me. Is there prior art to how to specify the ADMGPU target for an llvm-objcopy-like tool? The existing test shows that you can convert from one target (EM_NONE in the original test) to another target using the -O option, but the new test you've written doesn't demonstrate this behaviour (if I'm not mistaken, if you omitted the -O option, you'd get the same output, as the output target is derived from the input if not otherwise specified). Questions that I have are 1) if the input target was EM_AMDGPU and somebody specified e.g. -O elf32-hexagon, what should happen to any AMDGPU EF_* flag? 2) Similarly, how would you get a different AMDGPU flag? In other words, how would you get llvm-objcopy to add the EF_AMDGPU_MACH_* flag if it wasn't already present?

I'm wondering if a more natural solution to resolve the above questions is to, rather than support -O elf64-amdgpu is to support a family of values like -O elf64-amdgpu-amdgcn-gfx900. llvm-objcopy would then derive the appropriate EF_* and EM_* values from that. Similarly, it would learn to clear EF_* flags that aren't applicable. This would be new logic, as I don't believe currently llvm-objcopy does anything with ELF header flags. It would probably need to preserve non-machine-specific flags, and clear the other ones, similar to how it handles section header flags. WDYT?

llvm/test/tools/llvm-objcopy/ELF/amdgpu-cross-arch-headers.test

+# Show that the --output-format correctly configures both the main output file
+# and DWO output.
+# Note that we don't actually need any DWARF to produce the DWO file.
+
+# RUN: yaml2obj %s -o %t.o
+
+# Without --output-format, the format should match the input.
+
+# RUN: llvm-objcopy %t.o -O elf64-amdgpu %t.elf64_amdgpu.o --split-dwo=%t.elf64_amdgpu.dwo
+# RUN: llvm-readobj --file-headers %t.elf64_amdgpu.o | FileCheck %s --check-prefixes=CHECK,LE,AMDGPU,64,SYSV
+# RUN: llvm-readobj --file-headers %t.elf64_amdgpu.dwo | FileCheck %s --check-prefixes=CHECK,LE,AMDGPU,64,SYSV
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  # Arbitrary values that do not match any value we convert to via --output-format.
+  Machine:         EM_AMDGPU
+  OSABI:           ELFOSABI_STANDALONE
+  Flags:           [EF_AMDGPU_MACH_AMDGCN_GFX900]
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+Symbols:
+  - Name:     foo
+    Type:     STT_FUNC
+    Section:  .text
+    Value:    0x1234
+    Binding:  STB_GLOBAL
+  - Name:     bar
+    Type:     STT_OBJECT
+    Section:  .data
+    Value:    0xabcd
+    Binding:  STB_GLOBAL
+
+# CHECK:             Format:
+# AMDGPU-SAME:       elf64-amdgpu
+
+# AMDGPU-NEXT:       Arch: amdgcn
+
+# 64-NEXT:           AddressSize: 64bit
+
+# 64:                Class: 64-bit
+# LE:                DataEncoding: LittleEndian
+
+# SYSV:              OS/ABI: SystemV (0x0)
+
+# AMDGPU:            Machine: EM_AMDGPU (0xE0)
+
+# 64:                HeaderSize: 64
+
+# 64:                SectionHeaderEntrySize: 64

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

struct TargetInfo {
   FileFormat Format;
   MachineInfo Machine;
 };
 } // namespace
 
 // FIXME: consolidate with the bfd parsing used by lld.
 static const StringMap<MachineInfo> TargetMap{
     // Name, {EMachine, 64bit, LittleEndian}
     // x86
     {"elf32-i386", {ELF::EM_386, false, true}},
     {"elf32-x86-64", {ELF::EM_X86_64, false, true}},
     {"elf64-x86-64", {ELF::EM_X86_64, true, true}},
     // Intel MCU
     {"elf32-iamcu", {ELF::EM_IAMCU, false, true}},
     // ARM
     {"elf32-littlearm", {ELF::EM_ARM, false, true}},
     // ARM AArch64
     {"elf64-aarch64", {ELF::EM_AARCH64, true, true}},
static const StringMap<MachineInfo> TargetMap{
     // SPARC
     {"elf32-sparc", {ELF::EM_SPARC, false, false}},
     {"elf32-sparcel", {ELF::EM_SPARC, false, true}},
     // Hexagon
     {"elf32-hexagon", {ELF::EM_HEXAGON, false, true}},
     // LoongArch
     {"elf32-loongarch", {ELF::EM_LOONGARCH, false, true}},
     {"elf64-loongarch", {ELF::EM_LOONGARCH, true, true}},
+    // AMDGPU
+    {"elf64-amdgpu", {ELF::EM_AMDGPU, true, true}},
 };
 
 static Expected<TargetInfo>
 getOutputTargetInfoByTargetName(StringRef TargetName) {
   StringRef OriginalTargetName = TargetName;
   bool IsFreeBSD = TargetName.consume_back("-freebsd");
   auto Iter = TargetMap.find(TargetName);
   if (Iter == std::end(TargetMap))