This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Emit MessagePack HSA Metadata for v3 code object
ClosedPublic

Authored by scott.linder on Jun 14 2018, 10:02 AM.

Details

Summary

Continue to present HSA metadata as YAML in ASM and when output by tools (e.g. llvm-readobj), but encode it in Messagepack in the code object.

Diff Detail

Event Timeline

scott.linder created this revision.Jun 14 2018, 10:02 AM
arsenm added inline comments.Jun 14 2018, 1:56 PM
include/llvm/BinaryFormat/AMDGPUMetadataVerifier.h
63

Just use msgpack::Node&?

Address feedback

scott.linder added inline comments.Jun 15 2018, 10:38 AM
lib/Target/AMDGPU/MCTargetDesc/AMDGPUHSAMetadataStreamer.cpp
800–805 ↗(On Diff #151521)

@arsenm This calculation of the kernarg offset is analogous to what we used to have. We emitted DataLayout::getABITypeAlignment as "Align", and the runtime calculated the Offset.

Is DataLayout::getABITypeAlignment incorrect here? What is the correct way to reproduce the offsets calculated in the kernarg lowering?

Rebase and ping

nhaehnle added inline comments.Jul 31 2018, 3:24 AM
include/llvm/Support/AMDGPUMetadata.h
455

This is admittedly a bit of bike-shedding, but what's the rationale for outlining the strings in this way?

As a general rule, I think it's better to have constant strings inline because it keeps the code base more easily discoverable. If I run into an ELF where something is wrong with .name, then inlined strings allow me to just grep for ".name" and find all the relevant pieces of code immediately. With outlining, there's a level of indirection. I can find this location here easily, but then how do I reliably find the places where it's actually used? In this particular case, I can be reasonably confident that nobody is crazy enough to do using namespace Key, so a grep for Key::Name should do the trick, but it's still an extra step of indirection.

Address feedback. There was no particular reason for not writing these inline, other than to prevent typos. With the verifier and tests we have I don't think that is an issue, and I agree these being grepable is worthwhile.

Rebase and update:

  • Rename asm directive for V3 metadata
  • Avoid emitting old note types for V3 code objects
  • Fix LLVMBuild dependencies for libraries
  • Update NT_AMDGPU_METADATA note type value
kzhuravl requested changes to this revision.Nov 8 2018, 12:42 PM

Can you also rebase your change? I have done some changes to your patch in amd-common that you need to incorporate here.

tools/llvm-readobj/ELFDumper.cpp
3775–3796

Switch on a NoteType, then if the NoteType is ELF::NT_AMDGPU_METADATA interpret it.

This revision now requires changes to proceed.Nov 8 2018, 12:42 PM
scott.linder marked an inline comment as done.

Rebase tools/llvm-readobj/ELFDumper.cpp

Add missing AMDGPUNote type

Update getAMDGPUNote param name to match its new type

what is the rationale for putting the metadata verifier in binary format instead of support?

lib/Target/AMDGPU/AMDGPUAsmPrinter.h
59

unique_ptr?

what is the rationale for putting the metadata verifier in binary format instead of support?

It is a layering issue; BinaryFormat depends on Support, and the low-level MsgPack implementation is defined in BinaryFormat, so nothing that depends on it can live in Support.

Use unique_ptr for HSAMetadataStreamer

Rebase, ping

This revision is now accepted and ready to land.Dec 11 2018, 9:27 PM
This revision was automatically updated to reflect the committed changes.