Page MenuHomePhabricator

[AMDGPU] Factored PAL metadata handling out into its own class
Needs ReviewPublic

Authored by tpr on Jan 21 2019, 9:46 AM.

Details

Summary

This commit introduces a new AMDGPUPALMetadata class that:

  • is inside the AMDGPU target;
  • keeps an in-memory representation of PAL metadata;
  • provides a method for use by LLPC to write metadata into LLVM IR;
  • provides a method to read the frontend-supplied metadata from LLVM IR;
  • provides methods for the asm printer to set metadata items;
  • provides methods to write the metadata as a binary blob to put in a .note record or as an asm directive;
  • provides a method to read the metadata as a binary blob from a .note record.

Because llvm-readobj cannot call directly into a target, I had to remove
llvm-readobj's ability to dump PAL metadata, pending a resolution to
https://reviews.llvm.org/D52821

Change-Id: I756dc830894fcb6850324cdcfa87c0120eb2cf64

Diff Detail

Event Timeline

tpr created this revision.Jan 21 2019, 9:46 AM

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.

Herald added a project: Restricted Project. · View Herald TranscriptFri, Feb 1, 1:48 PM
scott.linder added inline comments.Fri, Feb 1, 1:55 PM
lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
85

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.

arsenm added inline comments.Fri, Feb 1, 1:57 PM
lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
85

They are

tpr added a comment.Mon, Feb 4, 9:50 AM

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.

tpr marked an inline comment as done.Mon, Feb 4, 9:53 AM
tpr added inline comments.
lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
85

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?

In D57027#1383432, @tpr 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.

lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
85

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.