[PATCH] D57028: [AMDGPU] Added MsgPack format PAL metadata

[Tim Renouf via Phabricator via llvm-commits]

tpr marked an inline comment as done.
tpr added inline comments.


================
Comment at: lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp:51-56
+  std::string Blob;
+  toMsgPackBlob(Blob);
+  auto MDS = MDString::get(M->getContext(), Blob);
+  auto Node = MDNode::get(M->getContext(), MDS);
+  auto Named = M->getOrInsertNamedMetadata("amdgpu.pal.metadata.msgpack");
+  Named->addOperand(Node);
----------------
scott.linder wrote:
> After thinking more about this and talking to @t-tye and others, I think it might be worth considering other ways to convey metadata through IR. We might also be able to avoid the issue of the front-end needing to make calls into the back-end if we just agree on a scheme and document it; if there is a contract between the front-end and back-end then you can safely put all of the code in the AMDGPU back-end without the private-headers issue.
> 
> I'm not sure I like the non-human-readable-blob approach. Since we have the YAML-Msgpack interop could this be a YAML string? I think this is something we would like to incorporate for HSA as well, and one requirement would be that it is readable and editable in IR.
Are you thinking we'd put the YAML string into an IR metadata string? I was thinking about that for a completely different application, but it looks to me like an IR string containing multiple lines just gets jammed into a single line with escapes for the newlines, so it is still not very editable.

Where we (in the PAL metadata world) have a requirement for editability is when the ELF is disassembled and reassembled, and that works fine with these changes because the PAL metadata is disassembled as a YAML block of text.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57028/new/

https://reviews.llvm.org/D57028