This is a basic implementation that allows lld to emit binaries consumable
by the HSA runtime.
Details
Diff Detail
Event Timeline
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. |
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. |
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. |
LGTM, but please wait for Simon's LGTM.
lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp | ||
---|---|---|
41 | Remove this "using". |
These methods do nothing now. I suggest to remove them from this commit and add later if necessary.