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

[Konstantin Zhuravlyov via llvm-commits]

kzhuravl added a comment.

Minor formatting + question



================
Comment at: lib/Target/AMDGPU/AMDGPUPTNote.h:29-38
+    NT_AMDGPU_HSA_CODE_OBJECT_VERSION = 1,
+    NT_AMDGPU_HSA_HSAIL = 2,
+    NT_AMDGPU_HSA_ISA = 3,
+    NT_AMDGPU_HSA_PRODUCER = 4,
+    NT_AMDGPU_HSA_PRODUCER_OPTIONS = 5,
+    NT_AMDGPU_HSA_EXTENSION = 6,
+    NT_AMDGPU_HSA_RUNTIME_METADATA = 7,
----------------
This might have been formatted incorrectly? Seems like '};' at column 2, but 'enum NoteType' at column 0?


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:496
+
+void AMDGPUTargetStreamer::emitRuntimeMetadataAsNoteElement(Module &M) {
+  auto &S = getStreamer();
----------------
yaxunl wrote:
> 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.
> 
Shouldn't we define pure virtual functions in base class, only implement them in AMDGPUTargetELFStreamer subclass, and possibly define same functions in AMGGPUTargetASMStreamer subclass but leave the body empty?


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:363
+      emitRuntimeMDIntValue(RuntimeMD::KeyArgPointeeAlign,
+                        DL.getABITypeAlignment(ET), 4);
+  }
----------------
Formatting


https://reviews.llvm.org/D25781