[PATCH] D11267: LLD: Add AMDGPU ELF ReaderWriter

[Simon Atanasyan]

atanasyan added a subscriber: atanasyan.
atanasyan added a comment.

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


================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUHSA.h:16
@@ +15,3 @@
+
+enum {
+  PT_AMDGPU_HSA_LOAD_GLOBAL_PROGRAM = PT_LOOS,
----------------
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.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.cpp:25
@@ +24,3 @@
+    : ELFLinkingContext(triple, llvm::make_unique<AMDGPUTargetHandler>(*this))
+    {}
+
----------------
Run the clang-format on this line.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.cpp:36
@@ +35,3 @@
+
+void setAMDGPUELFHeader(ELFHeader<ELF32LE> &elfHeader) { }
+void setAMDGPUELFHeader(ELFHeader<ELF64LE> &elfHeader) {
----------------
Do you really need this empty function?

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUSymbolTable.cpp:22
@@ +21,3 @@
+void AMDGPUSymbolTable::addDefinedAtom(Elf_Sym &sym, const DefinedAtom *da,
+                                    int64_t addr) {
+  SymbolTable::addDefinedAtom(sym, da, addr);
----------------
Run the clang-format on this line.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp:72
@@ +71,3 @@
+
+#if 0
+void AMDGPUTargetLayout::assignVirtualAddress() {
----------------
Do you need this commented code?

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp:77
@@ +76,3 @@
+  // FIXME: Is this the correct way to handle this?
+
+
----------------
Redundant empty line.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.cpp:83
@@ +82,3 @@
+	}
+
+}
----------------
Redundant empty line.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUTargetHandler.h:35
@@ +34,3 @@
+  void assignSectionsToSegments() override;
+//  void assignVirtualAddress() override;
+
----------------
Please remove the commented code.


http://reviews.llvm.org/D11267