[PATCH] D25781: AMDGPU: Emit runtime metadata as a note element in .note section

[Yaxun Liu via llvm-commits]

yaxunl marked 6 inline comments as done.
yaxunl added a comment.



> This is the right direction, but I think we should discuss this in more details. There are a few files that need to be shared between compiler and runtime (some elf stuff, amd_kernel_code_t, amd_queue_t? amd_signal_t?). Creating a file specifically for note stuff seems like an overkill to me IMHO.
> 
> I cleaned up files that are shared between compiler and runtime files a while ago, they are here:
> 
> - https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/amd_hsa_common.h
> - https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/amd_hsa_elf.h
> - https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/amd_hsa_kernel_code.h
> - https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/amd_hsa_queue.h
> - https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/amd_hsa_signal.h
> 
>   They still need more clean up since we switched to code object v2+. But maybe we can take those as a base.

I agree. Since it will take time and efforts to clean up and incorporate these header files, I would suggest to leave it to another patch.



================
Comment at: lib/Target/AMDGPU/AMDGPUPTNote.h:26
+
+const char NoteName[] = "AMD";
+
----------------
kzhuravl wrote:
> AMD\0
This is unnecessary since the literal string "AMD" has an implied trailing 0 and sizeof(NoteName) will include that, i.e., sizeof(NoteName)==4.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:496
+
+void AMDGPUTargetStreamer::emitRuntimeMetadataAsNoteElement(Module &M) {
+  auto &S = getStreamer();
----------------
kzhuravl wrote:
> I think ELF streaming belongs to AMDGPUTargetELFStreamer?
Normally I should define virtual functions for streaming the runtime metadata in textual and binary form separately in AMGGPUTargetASMStreamer and AMDGPUTargetELFStreamer. However we will replace this runtime metadata format with YAML based runtime metadata format soon, so I feel not worth the efforts to implement the textual format for the current runtime metadata format. After we move to YAML based runtime metadata format, I will come back to a better solution.



https://reviews.llvm.org/D25781