[AMDGPU] Add AMDGPU support for llvm-objcopy #92066

aakanksha555 · 2024-05-14T05:10:46Z

https://reviews.llvm.org/D143539

llvmbot · 2024-05-14T05:11:14Z

@llvm/pr-subscribers-objectyaml
@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-backend-amdgpu

Author: Aakanksha Patil (aakanksha555)

Changes

https://reviews.llvm.org/D143539

Full diff: https://github.com/llvm/llvm-project/pull/92066.diff

2 Files Affected:

(added) llvm/test/tools/llvm-objcopy/ELF/amdgpu-cross-arch-headers.test (+57)
(modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+2)

diff --git a/llvm/test/tools/llvm-objcopy/ELF/amdgpu-cross-arch-headers.test b/llvm/test/tools/llvm-objcopy/ELF/amdgpu-cross-arch-headers.test
new file mode 100644
index 0000000000000..a7242385950a8
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/amdgpu-cross-arch-headers.test
@@ -0,0 +1,57 @@
+# 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
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index a1897334cff2e..0fd1f9eac40f4 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -319,6 +319,8 @@ static const StringMap<MachineInfo> TargetMap{
     {"elf64-loongarch", {ELF::EM_LOONGARCH, true, true}},
     // SystemZ
     {"elf64-s390", {ELF::EM_S390, true, false}},
+    // AMDGPU
+    {"elf64-amdgpu", {ELF::EM_AMDGPU, true, true}},
 };
 
 static Expected<TargetInfo>

jh7370

This PR appears to have the same issues as I highlighted in the original Phabricator review. Please see https://reviews.llvm.org/D143539#4527049.

kzhuravl · 2024-05-14T16:20:29Z

Hi @Pierre-vh, can you help with the questions/issues in https://reviews.llvm.org/D143539#4527049

Pierre-vh · 2024-05-15T07:04:51Z

I agree that this should probably go in cross-arch-headers.test alongside the other targets, unless there is a good reason not to.

That being said, I'm not sure I agree with having the GPU name in the -O flag unless it's done dynamically. I'd like to avoid enumerating all GPUs in yet another place if it can be avoided. So either populate the map automatically, or parse the target into a flag, etc.

aakanksha555 · 2024-06-11T16:40:27Z

Hi, considering both of your feedback I'm trying to have amdgpu go into cross-arch-headers.test with a flag as suggested.

But, adding "Flags: [[FLAGS=<none>]]" to the FileHeader: gives-
YAML:136:20: error: expected sequence of bit values
Flags: <none>

Any suggestions on how I can go about incorporating the flag?

Pierre-vh · 2024-06-12T05:37:31Z

Hi, considering both of your feedback I'm trying to have amdgpu go into cross-arch-headers.test with a flag as suggested.

But, adding "Flags: [[FLAGS=]]" to the FileHeader: gives- YAML:136:20: error: expected sequence of bit values Flags:

Any suggestions on how I can go about incorporating the flag?

What happens if you use another default value for the flag, like 0?
I assume it's expecting some identifier or numbers, but it sees <none> which may not be valid YAML

Can you just update this patch with the cross-arch-headers.test instead, even if the test is broken? Just put a comment with the error. It'll be much easier to review this way

Add amdgpu support in cross-arch-headers.test instead of its own test

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

Pierre-vh · 2024-06-19T07:06:14Z

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

 # Note that we don't actually need any DWARF to produce the DWO file.

 # RUN: yaml2obj %s -o %t.o
+# RUN: yaml2obj -DFLAGS=[EF_AMDGPU_MACH_AMDGCN_GFX900] %s -o %t.o


You're overwriting the older file here which is a bigger issue
I now agree a separate test file is needed, so feel free to revert to the previous approach

I now agree a separate test file is needed, so feel free to revert to the previous approach

Please don't.

jh7370

Please could you use meaningful commit descriptions for fixups describing what has changed, rather than just repeating the PR title.

Let's go back to this conversation though:
(From Phabricator)

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 don't think these questions have been addressed at all. What is your proposal for this?

jh7370 · 2024-06-19T07:47:02Z

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

 # Note that we don't actually need any DWARF to produce the DWO file.

 # RUN: yaml2obj %s -o %t.o
+# RUN: yaml2obj -DFLAGS=[EF_AMDGPU_MACH_AMDGCN_GFX900] %s -o %t.o


I now agree a separate test file is needed, so feel free to revert to the previous approach

Please don't.

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

Pierre-vh

Please also add a test in binary-output-target.test

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

aakanksha555 · 2025-06-03T19:40:17Z

Hi,
Would it be okay to add -DELF_FLAGS to the yaml2obj invocations, it will be set to "" for every command invocation, except for AMDGPU, where we set it to EF_AMDGPU_MACH_AMDGCN_GFX900?

Pierre-vh · 2025-06-04T08:06:18Z

Hi, Would it be okay to add -DELF_FLAGS to the yaml2obj invocations, it will be set to "" for every command invocation, except for AMDGPU, where we set it to EF_AMDGPU_MACH_AMDGCN_GFX900?

I think that works, @jh7370 ?

jh7370 · 2025-06-04T09:02:50Z

Hi, Would it be okay to add -DELF_FLAGS to the yaml2obj invocations, it will be set to "" for every command invocation, except for AMDGPU, where we set it to EF_AMDGPU_MACH_AMDGCN_GFX900?

I think that works, @jh7370 ?

Functionally, it works, but I'd still prefer a yaml2obj enhancement to accept the "<none>" value for flags, as with other fields, as it will be cleaner in the test. I experimented with this and the change required is simple. Essentially, in the YAML code, you just need to make the header flags field optional and remove the default value from the mapOptional call that otherwise sets it. Then, when the value is used, you assign it a default value (of 0), if the optional field is unset. 2bfaf19 is an example of how this was done for the Link field in sections.

aakanksha555 · 2025-06-04T17:52:32Z

Hi, Would it be okay to add -DELF_FLAGS to the yaml2obj invocations, it will be set to "" for every command invocation, except for AMDGPU, where we set it to EF_AMDGPU_MACH_AMDGCN_GFX900?

I think that works, @jh7370 ?

Functionally, it works, but I'd still prefer a yaml2obj enhancement to accept the "" value for flags, as with other fields, as it will be cleaner in the test. I experimented with this and the change required is simple. Essentially, in the YAML code, you just need to make the header flags field optional and remove the default value from the mapOptional call that otherwise sets it. Then, when the value is used, you assign it a default value (of 0), if the optional field is unset. 2bfaf19 is an example of how this was done for the Link field in sections.

Got it, thanks for the feedback. Looking into it!

jh7370

There's a problem with obj2yaml now, which should be fairly trivial to fix. Also, could you split off the yaml2obj/obj2yaml changes into a separate PR, that this one relies on, please? It's functionally useful on its own.

jh7370 · 2025-06-05T06:30:39Z

llvm/test/Object/obj2yaml.test

 # ELF-X86-64-NEXT:   OSABI:           ELFOSABI_GNU
 # ELF-X86-64-NEXT:   Type:            ET_REL
 # ELF-X86-64-NEXT:   Machine:         EM_X86_64
+# ELF-X86-64-NEXT:   Flags:           [ ]


This is an undesirable outcome: if the Flags are empty, we shouldn't end up with a Flags entry in the output.

aakanksha555 · 2025-06-12T07:12:04Z

There's a problem with obj2yaml now, which should be fairly trivial to fix. Also, could you split off the yaml2obj/obj2yaml changes into a separate PR, that this one relies on, please? It's functionally useful on its own.

#143845

Split up the "FLAGS=" changes in the above PR

…3845) #92066 will be dependent on this change

… field (#143845) llvm/llvm-project#92066 will be dependent on this change

…m#143845) llvm#92066 will be dependent on this change

jh7370

Thank you for the yaml2obj improvements. This PR now looks a lot cleaner. However, I've still yet to see responses to my queries referenced in this comment. Please take a look and respond.

jh7370 · 2025-08-05T09:06:01Z

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

 # RUN: llvm-readobj --file-headers %t.elf64_s390.o | FileCheck %s --check-prefixes=CHECK,BE,S390X,64,SYSV
 # RUN: llvm-readobj --file-headers %t.elf64_s390.dwo | FileCheck %s --check-prefixes=CHECK,BE,S390X,64,SYSV

+


Nit: remove duplicate blank line.

jh7370 · 2025-08-05T09:08:45Z

llvm/test/tools/obj2yaml/ELF/mips-eflags.yaml

  Type:    ET_REL
  Machine: EM_MIPS
-  Flags:   [ ]
+  Flags: [ ]


Please reinstate the indentation so that the value part lines up with the other fields.

[AMDGPU] Add AMDGPU support for llvm-objcopy

07f69db

llvmbot added backend:AMDGPU llvm:binary-utilities labels May 14, 2024

arsenm approved these changes May 14, 2024

View reviewed changes

jh7370 requested changes May 14, 2024

View reviewed changes

kzhuravl requested a review from Pierre-vh May 14, 2024 16:19

Merge branch 'llvm:main' into main

130d3ea

[AMDGPU] Add AMDGPU support for llvm-objcopy

03fad14

Add amdgpu support in cross-arch-headers.test instead of its own test

aakanksha555 commented Jun 18, 2024

View reviewed changes

llvm/test/tools/llvm-objcopy/ELF/cross-arch-headers.test Show resolved Hide resolved

Pierre-vh reviewed Jun 19, 2024

View reviewed changes

jh7370 reviewed Jun 19, 2024

View reviewed changes

Pierre-vh reviewed Oct 25, 2024

View reviewed changes

llvm/test/tools/llvm-objcopy/ELF/cross-arch-headers.test Show resolved Hide resolved

aakanksha555 added 4 commits November 21, 2024 09:55

Merge branch 'llvm:main' into main

3cdf8e4

Merge branch 'llvm:main' into main

c987c64

Merge branch 'llvm:main' into main

fbf3ab2

Merge branch 'llvm:main' into main

e8f3278

Update cross-arch-headers.test

5fb376c

aakanksha555 requested a review from kzhuravl June 3, 2025 23:14

Allow ELF Fileheader Flags to accept "<none>" value

5c078b0

llvmbot added the objectyaml label Jun 5, 2025

jh7370 reviewed Jun 5, 2025

View reviewed changes

Merge branch 'llvm:main' into main

d876dd4

aakanksha555 mentioned this pull request Jun 12, 2025

Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags field #143845

Merged

chinmaydd self-requested a review June 18, 2025 21:07

aakanksha555 added a commit that referenced this pull request Jul 21, 2025

Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags field (#14…

0b054e2

…3845) #92066 will be dependent on this change

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jul 21, 2025

Automerge: Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags…

74fcbcb

… field (#143845) llvm/llvm-project#92066 will be dependent on this change

Merge branch 'llvm:main' into main

6c909a0

mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025

Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags field (llv…

fa13d3e

…m#143845) llvm#92066 will be dependent on this change

aakanksha555 deleted the branch llvm:main July 29, 2025 06:01

aakanksha555 closed this Jul 29, 2025

aakanksha555 deleted the main branch July 29, 2025 06:01

aakanksha555 restored the main branch July 29, 2025 06:12

aakanksha555 reopened this Jul 29, 2025

aakanksha555 and others added 5 commits July 31, 2025 14:50

Merge branch 'main' into main

44428e1

Merge branch 'main' into main

647ac77

Merge branch 'llvm:main' into main

4f940b4

Undo changes to the tests

45decf6

Undo changes to mips-eflags test

bdb7d74

jh7370 reviewed Aug 5, 2025

View reviewed changes

		# RUN: llvm-readobj --file-headers %t.elf64_s390.o \| FileCheck %s --check-prefixes=CHECK,BE,S390X,64,SYSV
		# RUN: llvm-readobj --file-headers %t.elf64_s390.dwo \| FileCheck %s --check-prefixes=CHECK,BE,S390X,64,SYSV

[AMDGPU] Add AMDGPU support for llvm-objcopy #92066

Are you sure you want to change the base?

[AMDGPU] Add AMDGPU support for llvm-objcopy #92066

Uh oh!

Conversation

aakanksha555 commented May 14, 2024

Uh oh!

llvmbot commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

kzhuravl commented May 14, 2024

Uh oh!

Pierre-vh commented May 15, 2024

Uh oh!

aakanksha555 commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierre-vh commented Jun 12, 2024

Uh oh!

Uh oh!

Pierre-vh Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

jh7370 Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-vh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aakanksha555 commented Jun 3, 2025

Uh oh!

Pierre-vh commented Jun 4, 2025

Uh oh!

jh7370 commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aakanksha555 commented Jun 4, 2025

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

aakanksha555 commented Jun 12, 2025

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

jh7370 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

llvmbot commented May 14, 2024 •

edited

Loading

aakanksha555 commented Jun 11, 2024 •

edited

Loading

jh7370 commented Jun 4, 2025 •

edited

Loading