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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D57027#1383432 <https://reviews.llvm.org/D57027#1383432>, @tpr wrote:

> In D57027#1381468 <https://reviews.llvm.org/D57027#1381468>, @scott.linder wrote:
>
> > The code looks good to me, but I'm wondering why the move from doing this in `AsmPrinter::EmitEndOfAsmFile` to `TargetStreamer::finish`? It seems like it is fine to emit directives in either, but I don't understand why the change here.
>
>
> The idea was so that a disassembler disassembling into a TargetStreamer would go through the same code to emit the directives.


That sounds reasonable to me, I was just curious if HSA should move where it emits the equivalent metadata note to the streamer as well.



================
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);
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57027