This is an archive of the discontinued LLVM Phabricator instance.

LLD: Add AMDGPU ELF ReaderWriter
ClosedPublic

Authored by tstellarAMD on Jul 16 2015, 8:00 AM.

Details

Summary

This is a basic implementation that allows lld to emit binaries consumable
by the HSA runtime.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to LLD: Add AMDGPU ELF ReaderWriter.
tstellarAMD updated this object.
tstellarAMD added a subscriber: llvm-commits.
tstellarAMD added a subscriber: lld.

As to me I would add more tests. For example, check ELF file header fields,

lib/ReaderWriter/ELF/AMDGPU/AMDGPUHSA.h
16 ↗(On Diff #29912)

I think it is better to move all these declarations to llvm\Support\ELF.h. In that case you can support these flags in the llvm-readobj tool.

lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.cpp
25 ↗(On Diff #29912)

Run the clang-format on this line.

36 ↗(On Diff #29912)

Do you really need this empty function?

lib/ReaderWriter/ELF/AMDGPU/AMDGPUSymbolTable.cpp
22 ↗(On Diff #29912)

Run the clang-format on this line.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp
72 ↗(On Diff #29912)

Do you need this commented code?

77 ↗(On Diff #29912)

Redundant empty line.

83 ↗(On Diff #29912)

Redundant empty line.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.h
35 ↗(On Diff #29912)

Please remove the commented code.

Here is an updated patch with the following changes:

  • Ran clang-format on all new files.
  • Merged http://reviews.llvm.org/D11265 into this patch.
  • Moved AMDGPU ELF definitions into Support/ELF.h.
  • Added more checks in the testcase.

I added some notes and included Rui Ueyama to the reviewers list.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUExecutableWriter.cpp
18–33 ↗(On Diff #30477)

These methods do nothing now. I suggest to remove them from this commit and add later if necessary.

lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.h
23 ↗(On Diff #30477)

Usually we put constructors in the beginning of the class declarations. Other methods go after that.

31 ↗(On Diff #30477)

Do you really need this function declaration? Below in the AMDGPULinkingContext.cpp you define void setAMDGPUELFHeader(ELFHeader<ELF64LE> &elfHeader) only.

ruiu added inline comments.Aug 3 2015, 3:07 PM
lib/ReaderWriter/ELF/AMDGPU/AMDGPUELFFile.cpp
14–22 ↗(On Diff #30477)

Can you add llvm::ELF::SHT_NOTE to ELFFile::handleSectionWithNoSymbols and remove this function? I'm not 100% sure but I believe doing that is not harmful to other ports.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp
55 ↗(On Diff #30477)

Use real type instead of auto as the type is not obvious.

56 ↗(On Diff #30477)

Ditto

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.h
39 ↗(On Diff #30477)

Remove this empty line.

43–45 ↗(On Diff #30477)

You may want to move these lines above if (name == ".note") because this statement does not use contentType and returns immediately.

tstellarAMD marked 12 inline comments as done.Aug 5 2015, 11:39 AM
tstellarAMD added inline comments.
lib/ReaderWriter/ELF/AMDGPU/AMDGPUExecutableWriter.cpp
18–33 ↗(On Diff #30477)

These functions are actually calling the implementation of the parent class's parent instead of the parent's. This is done to avoid having c runtime symbols added to the binary.

Here is an updated patch.

tstellarAMD marked 2 inline comments as done.Aug 5 2015, 11:41 AM
ruiu accepted this revision.Aug 25 2015, 10:55 PM
ruiu edited edge metadata.

LGTM, but please wait for Simon's LGTM.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp
40 ↗(On Diff #31383)

Remove this "using".

This revision is now accepted and ready to land.Aug 25 2015, 10:55 PM
atanasyan accepted this revision.Aug 26 2015, 12:34 AM
atanasyan added a reviewer: atanasyan.

LGTM

This revision was automatically updated to reflect the committed changes.