This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] add AMDGPU target support to ELFObjectFile.h header
ClosedPublic

Authored by vpykhtin on Feb 11 2016, 8:58 AM.

Details

Summary

Allows to test Obj->getArch () == Triple::amdgpu and prints "ELF-amdgpu-hsacobj" on ELF dump.

Diff Detail

Repository
rL LLVM

Event Timeline

vpykhtin updated this revision to Diff 47656.Feb 11 2016, 8:58 AM
vpykhtin retitled this revision from to [AMDGPU] add AMDGPU target support to ELFObjectFile.h header.
vpykhtin updated this object.
vpykhtin added reviewers: tstellarAMD, arsenm.
vpykhtin set the repository for this revision to rL LLVM.
vpykhtin added a subscriber: nhaustov.
arsenm added inline comments.Feb 11 2016, 11:26 AM
include/llvm/Object/ELFObjectFile.h
848 ↗(On Diff #47656)

Why is the -unknown necessary?

Also should this be amdgcn? The other targets look like they are using the regular arch prefix here

vpykhtin added inline comments.Feb 11 2016, 11:59 AM
include/llvm/Object/ELFObjectFile.h
848 ↗(On Diff #47656)

Don't have any preference here, let it be amdgcn.

HSA Code Object spec has requirement :

EF.getHeader()->e_ident[ELF::EI_CLASS] == ELF::EM_AMDGPU
&& EF.getHeader()->e_ident[ELF::EI_OSABI] == ELF::ELFOSABI_AMDGPU_HSA
&& IsLittleEndian

all other I consider "unknown" though not sticking on it either.

vpykhtin updated this revision to Diff 47803.EditedFeb 12 2016, 7:54 AM
vpykhtin added a project: Restricted Project.

per review fixes.

vpykhtin marked 2 inline comments as done.Feb 12 2016, 7:55 AM
arsenm added inline comments.Feb 15 2016, 5:21 PM
include/llvm/Object/ELFObjectFile.h
848 ↗(On Diff #47803)

I would expect the enum name to change. Is this the one standardized somewhere? If we can't change the enum, then I guess the string should match

vpykhtin added inline comments.Feb 16 2016, 8:47 AM
include/llvm/Object/ELFObjectFile.h
848 ↗(On Diff #47803)

Well, EM_AMDGPU extensively used and match the name in the HSA Code Object spec so I change name to "ELFxx-amdgpu".

vpykhtin updated this revision to Diff 48074.Feb 16 2016, 8:48 AM

ELFxx-amdgpu name used.

vpykhtin marked an inline comment as done.Feb 16 2016, 8:48 AM

Kind reminder if someone can take a look at this.

Bigcheese edited edge metadata.Feb 29 2016, 6:12 PM

Two things. This needs tests, and i want to make sure that we use the same file format name as binutils.

vpykhtin updated this revision to Diff 49501.Mar 1 2016, 9:32 AM
vpykhtin edited edge metadata.
vpykhtin removed rL LLVM as the repository for this revision.

Two things. This needs tests, and i want to make sure that we use the same file format name as binutils.

Added tests.

Michael, this is the first place we introducing format name, what would make you sure it will be used in binutils? Should it be registered somewhere?

So, is this the same target that has been added to lld?

Before we continue adding support for it in llvm I want to make sure the address X offset issue is resolved.

Bigcheese accepted this revision.Mar 1 2016, 11:02 AM
Bigcheese edited edge metadata.

lgtm. This being the only place it's currently being added is good enough.

This revision is now accepted and ready to land.Mar 1 2016, 11:02 AM

Please don't.

What is currently being produced by lld is simply not valid elf. We
should not be spreading it before it is fixed.

Cheers,
Rafael

ruiu added a subscriber: ruiu.Mar 1 2016, 11:09 AM

I agree with Rafael. Some fields of the AMDGPU ELF file were defined based on a misunderstanding of the ELF file format. Please please fix it before it's too late.

arsenm edited edge metadata.Mar 1 2016, 11:23 AM
In D17144#365490, @ruiu wrote:

I agree with Rafael. Some fields of the AMDGPU ELF file were defined based on a misunderstanding of the ELF file format. Please please fix it before it's too late.

What's currently implemented now is a 1st draft of the spec.Soon a newer version will be implemented which I'm pretty sure fixes this issue

Can you fix it and then add code to LLVM? Adding code to LLVM allows people
to start creating object files in the odd "ELF" format. If a fix is not
made in a timely manner, it can be too late, and there's a chance that you
will have no choice other than supporting the old broken format.

Can you fix it and then add code to LLVM? Adding code to LLVM allows people
to start creating object files in the odd "ELF" format. If a fix is not
made in a timely manner, it can be too late, and there's a chance that you
will have no choice other than supporting the old broken format.

Is the concern more about custom non-standard code in lld for AMDGPU? We are now actively working on making it more standard.

This change is more about general support in LLVM for EM_AMDGPU and allows us to run llvm-objdump on AMDGPU ELFs to verify changes in disassembler. EM_AMDGPU is officially registered too, see http://www.sco.com/developers/gabi/latest/ch4.eheader.html

Assuming we work on removing non-standard code from lld, is there anything wrong with this change?

ruiu added a comment.Mar 2 2016, 9:06 AM

No, there's no particular concern regarding this change itself. I wanted to see it being removed until the AMDGPU support becomes generally available. Because the code for the nonstandard part of the ABI is being removed from lld, I'm fine with this change. Thank you for doing this.

This is fine by me now that the non-elf bits were removed from lld.

This revision was automatically updated to reflect the committed changes.