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.
Details
Diff Detail
Event Timeline
include/llvm/BinaryFormat/AMDGPUMetadataVerifier.h | ||
---|---|---|
63 | Just use msgpack::Node&? |
lib/Target/AMDGPU/MCTargetDesc/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
800–805 | @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? |
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
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 | ||
---|---|---|
3658–3679 | Switch on a NoteType, then if the NoteType is ELF::NT_AMDGPU_METADATA interpret it. |
what is the rationale for putting the metadata verifier in binary format instead of support?
lib/Target/AMDGPU/AMDGPUAsmPrinter.h | ||
---|---|---|
59 | unique_ptr? |
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.
Just use msgpack::Node&?