[PATCH] D143539: [AMDGPU] Add AMDGPU support for llvm-objcopy

Mon Jul 24 01:25:56 PDT 2023

jhenderson added a comment.

In D143539#4519733 <https://reviews.llvm.org/D143539#4519733>, @aakanksha555 wrote:

> In D143539#4517371 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143539