Skip to content

[Comgr][hotswap] Add code-object metadata parsing#2437

Open
tgymnich wants to merge 2 commits into
users/tgymnich/hotswap-pr-01-scaffoldingfrom
users/tgymnich/hotswap-pr-02-code-object-metadata
Open

[Comgr][hotswap] Add code-object metadata parsing#2437
tgymnich wants to merge 2 commits into
users/tgymnich/hotswap-pr-01-scaffoldingfrom
users/tgymnich/hotswap-pr-02-code-object-metadata

Conversation

@tgymnich
Copy link
Copy Markdown

@tgymnich tgymnich commented May 7, 2026

Adds the ELF/MsgPack metadata parser that the rest of the raiser pipeline
depends on. Public surface is extractTextSection, listKernelNames,
extractKernelMeta, findKernelSymbolOffset, and detectIsaFromElf.

No MC stack, no disassembler, no opcode canonicalisation yet — those land
in subsequent commits. The kernel-descriptor fields on KernelMeta (kernel
code properties, kernarg preload, compute_pgm_rsrc1/2) are populated here
so later layers (UserSgprLayout, kernarg layout) have the metadata they
expect; the raiser itself does not consume them yet.

Includes a small focused gtest covering the empty-input and malformed-ELF
guards on the public API.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

@tgymnich tgymnich requested review from chinmaydd and lamb-j as code owners May 7, 2026 16:12
@tgymnich tgymnich changed the title [Comgr][hotswap] Add bare-bones hotswap transpiler scaffolding [Comgr][hotswap] Add code-object metadata parsing May 7, 2026
@z1-cciauto
Copy link
Copy Markdown
Collaborator

@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from 81e6053 to d1a05c9 Compare May 7, 2026 16:23
@tgymnich
Copy link
Copy Markdown
Author

tgymnich commented May 7, 2026

@z1-cciauto
Copy link
Copy Markdown
Collaborator

@tgymnich tgymnich changed the base branch from amd-staging to users/tgymnich/hotswap-pr-01-scaffolding May 7, 2026 16:29
@chinmaydd chinmaydd added comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature labels May 7, 2026
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from 2675e92 to 3831ee0 Compare May 8, 2026 10:17
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from d1a05c9 to 8be9478 Compare May 8, 2026 10:17
@lamb-j
Copy link
Copy Markdown
Collaborator

lamb-j commented May 8, 2026

Thanks for putting this up. Two main pieces of feedback, both around the AGENT_CONVENTIONS "Comgr first, LLVM second, custom never" rule. The below is obv Claude's thoughts. But in general, we can beef up the existing Comgr metadata API implementations if we need to support Hotswap's needs. I think that could benefit both "sub-projects".


Reuse existing Comgr metadata APIs

amd/comgr/src/comgr-metadata.{h,cpp} already does most of what this PR adds, and reusing it would also fix a few correctness issues:

This PR Existing Comgr API
extractKernelMeta / listKernelNames (manual note walk + MsgPack parse) getMetadataRootgetElfMetadataRoot<ELFT> (comgr-metadata.cpp:190, 254)
detectIsaFromElf (e_machine → "gfx1250") getElfIsaNamegetElfIsaNameFromElfHeader<ELFT> (comgr-metadata.cpp:350, 410)
findKernelSymbolOffset symbol walk amd_comgr_iterate_symbols + amd_comgr_symbol_get_info, or the internal helpers in comgr-symbol.cpp

Switching to getElfMetadataRoot also fixes three latent bugs in the hand-rolled walker:

  1. Misses program-header notes. getElfMetadataRoot walks PT_NOTE program headers first, then SHT_NOTE sections. This PR only walks sections — fully-linked AMDGPU code objects often carry notes via program headers only, so they'd come back with zero kernels.
  2. Misses name=="AMD" / NT_AMD_HSA_METADATA (older YAML) and PAL metadata. The hand-rolled check is hardcoded to name=="AMDGPU" && type==32. processNote + mergeNoteRecords handle all three vendor-name/type combinations.
  3. Bails on anything that isn't ELF64LE. getElfObjectFileBase dispatches across ELF32LE / ELF64LE / ELF32BE / ELF64BE.

detectIsaFromElf similarly returns a bare "gfx1250" and drops OS ABI, ABI version, and the sramecc/xnack feature bits. getElfIsaNameFromElfHeader already produces the canonical amdgcn-amd-amdhsa--gfx1250:sramecc+:xnack- triple — depending on what the raiser does downstream with the ISA string, dropping the feature bits is likely a problem.

The Comgr internal helpers take DataObject* / DataMeta*, which a standalone transpiler library doesn't have. The minor update I'd propose is lifting getElfMetadataRoot and getElfIsaNameFromElfHeader to a MemoryBufferRef/ArrayRef<uint8_t> overload (the templates already take ELFObjectFile<ELFT>*, so the wrapping is thin), with the existing DataObject* entry points becoming one-line forwarders. Happy to do that update separately if it helps unblock this.

The one piece that's genuinely new is readKernelDescriptorBytes — the 64-byte KD blob isn't surfaced via MsgPack and the raiser legitimately needs it. Even there, the symbol-lookup half should go through amd_comgr_iterate_symbols / a comgr-symbol.cpp helper; only the "reinterpret the bytes as kernel_descriptor_t" part is net-new.

Reuse existing LLVM facilities

A few hand-rolled bits that already exist in llvm/:

  • readFile()llvm::MemoryBuffer::getFile (handles error, empty file, mmap)
  • readU32 / readU16 (host-endian memcpy) → llvm::support::endian::read32le / read16le (llvm/Support/Endian.h)
  • Manual ELF note iteration → llvm::object::ELFFile::notes() (Elf_Note_Iterator) — this also removes the ~50-line duplication between listKernelNames and extractKernelMeta
  • if (shdr.sh_type != 7) // SHT_NOTEllvm::ELF::SHT_NOTE
  • if (type == 32 ...)llvm::ELF::NT_AMDGPU_METADATA
  • 64 (KD size) → sizeof(llvm::amdhsa::kernel_descriptor_t), and ideally read fields via reinterpret_cast<kernel_descriptor_t*> rather than readU32(... + COMPUTE_PGM_RSRC1_OFFSET)

Most of these collapse into the Comgr reuse above (the note iteration disappears entirely if you go through getElfMetadataRoot).

@chinmaydd
Copy link
Copy Markdown

We really really really need to pull out common functionality that is shared between the transpiler and the translator into a singular module. We will have to suffer from a lot of code duplication later otherwise.

@tgymnich tgymnich marked this pull request as draft May 8, 2026 21:26
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from 3831ee0 to 9f6c2ed Compare May 11, 2026 11:40
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from 8be9478 to 6ab4011 Compare May 11, 2026 11:41
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from 9f6c2ed to ec69509 Compare May 11, 2026 12:32
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from 6ab4011 to dd6bb68 Compare May 11, 2026 12:32
@tgymnich tgymnich marked this pull request as ready for review May 11, 2026 12:40
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from ec69509 to 3b59a99 Compare May 11, 2026 12:42
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch 2 times, most recently from 94e24d0 to d6f97c5 Compare May 11, 2026 15:21
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from d6f97c5 to 2b8bfde Compare May 11, 2026 17:46
@lamb-j
Copy link
Copy Markdown
Collaborator

lamb-j commented May 11, 2026

For the new source files, should we use code-object-utils.cpp instead of code_object_utils.cpp? Not a defined convention and looks like we deviate from LLVM on this (which would use CodeObjectUtils.cpp). But probably best to follow existing practice

@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from 3b59a99 to 91c4a7e Compare May 11, 2026 20:21
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from 2b8bfde to 70e5099 Compare May 11, 2026 20:21
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from 91c4a7e to cceb980 Compare May 11, 2026 20:48
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from 70e5099 to 73d63ce Compare May 11, 2026 20:48
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-01-scaffolding branch from cceb980 to 7b8ccf2 Compare May 11, 2026 21:26
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch 2 times, most recently from 4314144 to 7fde156 Compare May 11, 2026 21:58
martin-luecke and others added 2 commits May 12, 2026 00:09
Lands the minimal scaffolding
needed to build a `hotswap-transpiler` static library:

  * `raiser.{hpp,cpp}` — `raiseToIR()` entry point. The implementation
    creates an `llvm::Module` with a single `ret void` AMDGPU kernel
    function for any input. ELF ingestion, MC disassembly and the
    per-format raise handlers land in subsequent patches.
  * `raise_failure.{hpp,cpp}` — structured failure values consumed by
    `RaiseResult::failure`.
  * `code_object_utils.h` — `KernelMeta` struct used in the
    `raiseToIR` signature.
  * Minimal `CMakeLists.txt` linking only `LLVMCore`, `LLVMSupport`,
    `LLVMTargetParser`. The full library/include surface grows
    incrementally as later patches need it.
  * `tests/raiser_empty_module_test.cpp` (gtest) verifies the
    scaffolding contract: an empty input produces a well-formed
    module with one `AMDGPU_KERNEL` function whose body is `ret void`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the AMDGPU code-object metadata extraction surface that the rest of
the raiser pipeline depends on:

* `extractTextSection`, `listKernelNames`, `extractKernelMeta`,
  `findKernelSymbolOffset`, and `detectIsaFromElf` in
  `hotswap/code_object_utils.{h,cpp}`.
* `KernelMeta` populated with both MsgPack-derived fields (kernarg/group/
  private segment sizes, args) and the raw kernel-descriptor register
  bytes (`compute_pgm_rsrc{1,2}`, `kernel_code_properties`,
  `kernarg_preload`) read from `<name>.kd` in `.rodata`. Later layers
  (`UserSgprLayout`, kernarg layout) consume these.

Reuses comgr's existing parsing infrastructure rather than duplicating
it. Two new `MemoryBufferRef`-friendly entry points are added to
`comgr-metadata.{h,cpp}` so hotswap can call them without going through
the `DataObject` / `DataMeta` ceremony of the public API:

* `metadata::walkElfMetadataIntoDoc(MemoryBufferRef, msgpack::Document&,
  bool& EmitIntegerBooleans)` — walks PT_NOTE program headers and
  SHT_NOTE sections, recognises `NT_AMD_HSA_METADATA` (YAML, name="AMD"),
  `NT_AMDGPU_METADATA` (MsgPack, name="AMDGPU"), and the PAL note
  (type 13). `getMetadataRoot(DataObject*, DataMeta*)` is reduced to a
  thin wrapper around it.
* `metadata::getElfIsaNameFromBuffer(MemoryBufferRef, std::string&)` —
  produces the canonical `amdgcn-amd-amdhsa--<gfx>:sramecc±:xnack±`
  string. `getElfIsaName(DataObject*, std::string&)` delegates.

The existing internal templates (`mergeNoteRecords`, `processElfNote`,
`walkElfMetadata`, `getElfIsaNameFromElfHeader`) stay in
`comgr-metadata.cpp`'s anonymous namespace as the single source of
truth.

Includes a small focused gtest covering empty-input and malformed-ELF
guards on the public hotswap API.

No MC stack, no disassembler, no opcode canonicalisation yet — those
land in subsequent commits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tgymnich tgymnich force-pushed the users/tgymnich/hotswap-pr-02-code-object-metadata branch from 7fde156 to 2c6f5a2 Compare May 11, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants