Page MenuHomePhabricator

LLD: Add AMDGPU ELF ReaderWriter

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



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,

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.

25 ↗(On Diff #29912)

Run the clang-format on this line.

36 ↗(On Diff #29912)

Do you really need this empty function?

22 ↗(On Diff #29912)

Run the clang-format on this line.

72 ↗(On Diff #29912)

Do you need this commented code?

77 ↗(On Diff #29912)

Redundant empty line.

83 ↗(On Diff #29912)

Redundant empty line.

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 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.

18–33 ↗(On Diff #30477)

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

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
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.

55 ↗(On Diff #30477)

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

56 ↗(On Diff #30477)


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.
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.

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.


This revision was automatically updated to reflect the committed changes.