This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Added MsgPack format PAL metadata
ClosedPublic

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

Details

Summary

PAL metadata now supports both the old linear reg=val pairs format and
the new MsgPack format.

The MsgPack format uses YAML as its textual representation. On output to
YAML, a mnemonic name is provided for some hardware registers.

Change-Id: I2bbaabaaca4b3574f7e03b80fbef7c7a69d06a94

Diff Detail

Event Timeline

tpr created this revision.Jan 21 2019, 9:48 AM
scott.linder added inline comments.Feb 8 2019, 8:14 AM
lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
51–56

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.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 8:14 AM
tpr marked an inline comment as done.Feb 10 2019, 11:04 AM
tpr added inline comments.
lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
51–56

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.

tpr marked an inline comment as done.Feb 11 2019, 1:57 AM
tpr added inline comments.
lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
51–56

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.
tpr updated this revision to Diff 191103.Mar 18 2019, 8:54 AM

V2: Like the previous commit, no longer supports a method for LLPC to

call to write the PAL metadata into IR metadata. The plan now is
that LLPC will use MsgPackDocument and put the msgpack binary blob
into IR metadata itself.
tpr updated this revision to Diff 191256.Mar 19 2019, 12:56 AM

V3: Rebased, and addressed "single char in single quotes" review comment from the other change.

scott.linder added inline comments.Mar 19 2019, 8:24 AM
include/llvm/BinaryFormat/ELF.h
1369

I thought the goal was to use NT_AMDGPU_METADATA = 32 for both HSA and PAL when we made the move to MsgPack; can we make this switch now?

tpr updated this revision to Diff 191482.Mar 20 2019, 5:19 AM

V4: Generate vendor=AMDGPU, type=32 for the metadata note record, as now expected by PAL.

tpr marked an inline comment as done.Mar 20 2019, 5:19 AM
scott.linder accepted this revision.Mar 20 2019, 7:57 AM

LGTM, thanks! One small change to a comment.

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

Can we drop ELF::NT_AMD_AMDGPU_PAL_METADATA_MSGPACK here?

This revision is now accepted and ready to land.Mar 20 2019, 7:57 AM
This revision was automatically updated to reflect the committed changes.