[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);
----------------
tpr wrote:
> 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.
Just to add to this: What swayed me to use a binary blob of msgpack:
1. This is used in an online compiler in a graphics device driver, so that made me think that a textual format, or something more complicated, is not suitable.
2. The contract is that the frontend provides this msgpack document, and LLVM mostly does not have to understand it, except that LLVM adds to it, and in a few cases ORs a value into an existing int value in the document tree if there is already one there. So LLVM does not fully understand the metadata part of the PAL ABI, only the bits it needs to add to.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57028