[PATCH] D57027: [AMDGPU] Factored PAL metadata handling out into its own class

Mon Mar 18 08:56:45 PDT 2019

tpr marked 3 inline comments as done.
tpr added inline comments.

================
Comment at: lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h:85
+  // Write the accumulated PAL metadata as LLVM IR metadata. This is used by the
+  // frontend to pass the PAL metadata to the backend.
+  void writeToIR(Module *M, bool MsgPackFormat);
----------------
scott.linder wrote:
> tpr wrote:
> > arsenm wrote:
> > > scott.linder wrote:
> > > > How is the front-end structured so that it has access to this header? I thought all of the headers in the AMDGPU target were private.
> > > They are
> > Right, I've got the frontend diving in and directly including the .h file from inside lib/Target/AMDGPU. Is that bad? Is that an argument for going back to putting this stuff, maybe excluding the parts used by codegen, in lib/BinaryFormat, or something?
> My understanding is that directly referencing private headers, like those in lib/Target/AMDGPU, is not correct. One immediate issue you might encounter is that the headers do not exist in the installed copy of LLVM, meaning you will only be able to compile against the LLVM build directory. I think you're right that this is an argument for moving anything intended for use outside of the backend to somewhere public.
LLPC no longer dives in to access the private header, and I have removed the API that LLPC was using.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57027