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

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

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

Run the clang-format on this line.

36

Do you really need this empty function?

lib/ReaderWriter/ELF/AMDGPU/AMDGPUSymbolTable.cpp
22

Run the clang-format on this line.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp
72

Do you need this commented code?

77

Redundant empty line.

83

Redundant empty line.

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.h
35

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
19–34

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

lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.h
24

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

32

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
15–23

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
56

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

57

Ditto

lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.h
40

Remove this empty line.

44–46

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
19–34

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
41

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.