This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Add AMDGPU support
ClosedPublic

Authored by tstellarAMD on Jan 5 2016, 10:38 AM.

Details

Summary

This will allow us to remove the AMDGPU support from old ELF.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to [ELF] Add AMDGPU support.
tstellarAMD updated this object.
tstellarAMD added reviewers: ruiu, rafael.
ruiu edited edge metadata.Jan 5 2016, 10:53 AM

This patch itself seems simple, but I don't know the reason of each change. A description for each change would be greatly appreciated.

ELF/Driver.cpp
246

Why is AMDGPU special for this if?

ELF/OutputSections.cpp
789–790

Why is this needed?

1295

Ditto

ELF/Target.cpp
1272

{ } -> {}

(Please run clang-format for this patch.)

ELF/Writer.cpp
840

unsigned -> uint32_t.

This function needs a brief comment to describe why AMDGPU is special.

tstellarAMD edited edge metadata.

Added comments explaining changes.

tstellarAMD marked 4 inline comments as done.Jan 5 2016, 11:48 AM
tstellarAMD marked an inline comment as done.Jan 5 2016, 11:49 AM
ruiu added inline comments.Jan 5 2016, 12:01 PM
ELF/Driver.cpp
248

Then the check should be added here, no?

ELF/OutputSections.cpp
789

It still looks odd since the value does not include section's address, but I guess there's a reason for that. Is there a spec or something for this? Please add a link if the document is available.

Clarify a comment and move EM_AMDGPU check.

tstellarAMD marked an inline comment as done.Jan 5 2016, 12:39 PM
tstellarAMD added inline comments.
ELF/OutputSections.cpp
789

I have clarified this comment in the latest revision, but offsets stored in a symbol's st_value field for AMDGPU need to be byte offsets from the start of the section. So the section address needs to be ignored when computing the offset.

There is a document specifying this object format. I have been working with people internally to get it released, I will add it to the documents once it is available.

ruiu added inline comments.Jan 5 2016, 12:48 PM
ELF/Driver.cpp
251

If -e doesn't make sense for AMDGPU, should we treat it an error if a user specifies -e for AMDGPU?

ELF/Writer.cpp
904–905

I think this is slightly better since readers don't have to read the subroutine unless they are interested in AMDGPU.

uint32_t PTType = (Config->Emachine != EM_AMDGPU) ? PT_LOAD : getAmdgpuPhdr();

Prevent -e from being used with AMDGPU, simplify some other code.

tstellarAMD marked 2 inline comments as done.Jan 6 2016, 8:36 AM
ruiu accepted this revision.Jan 6 2016, 11:59 AM
ruiu edited edge metadata.

LGTM

ELF/Writer.cpp
843

Remove this empty line.

847

Ditto

850

Ditto

This revision is now accepted and ready to land.Jan 6 2016, 11:59 AM
Closed by commit rL257023: [ELF] Add AMDGPU support (authored by tstellar). · Explain WhyJan 6 2016, 8:02 PM
This revision was automatically updated to reflect the committed changes.