[Comgr] Add end-to-end LIT coverage for amd_comgr_hotswap_rewrite by harsh-amd · Pull Request #2291 · ROCm/llvm-project

harsh-amd · 2026-04-22T17:09:59Z

Summary

End-to-end LIT coverage for amd_comgr_hotswap_rewrite. Drives the full compile -> hotswap-rewrite -> verify chain on a real clang-produced gfx1250 code object, using the in-tree %clang / %llvm-readelf / %llvm-objdump / %FileCheck substitutions -- no external toolchain required.

Follow-up to #2203. Implements part (A) of the testing-infrastructure plan laid out in that PR's comment thread: the dedicated infra-PR that goes in before any real-patch PR lands.

Changes

test-lit/hotswap-rewrite-e2e.hip (new): compiles a tiny kernel with %clang --offload-arch=gfx1250 --offload-device-only, pipes the resulting code object through hotswap-rewrite, and verifies the output with %llvm-readelf -h / %llvm-readelf --notes / %llvm-readelf --section-headers / %llvm-objdump -d.
test-lit/lit.cfg.py: add %llvm-readelf substitution (only %llvm-objdump was wired up previously).
test-lit/comgr-sources/hotswap-rewrite.c: optional --output <path> flag to write the rewrite output to a file so LIT tests can inspect it. Existing --zero-size negative-path coverage and the NULL-args / unsupported-ISA / malformed-ELF stanzas are unchanged.

What this covers today

Patches in comgr-hotswap-b0a0.cpp are weak stubs returning 0 in #2203, so the dispatcher currently emits an output that is bytewise-identical to the input. Even with no patches applied we assert:

Input is a gfx1250 ELF (e_flags contains gfx1250, AMDHSA metadata note records amdhsa.target: amdgcn-amd-amdhsa--gfx1250).
amd_comgr_hotswap_rewrite returns AMD_COMGR_STATUS_SUCCESS.
Output ELF preserves the gfx1250 identity on both channels (e_flags + AMDHSA note).
.text section is still present and PROGBITS.
llvm-objdump recognizes the output as elf64-amdgpu and disassembles through s_endpgm.

Future per-patch PRs

Each subsequent real-patch PR (in-place / trampoline / WMMA / scratch) will layer its own llvm-objdump | FileCheck stanza on top of this harness to assert the specific opcode changes / trampolines its policy introduces. No further infra PRs needed -- part (B) of the plan in #2203 is a per-PR convention rather than a dedicated PR.

Test plan

`llvm-lit tools/comgr/test-lit/hotswap-rewrite-e2e.hip` passes locally
Existing `tools/comgr/test-lit/hotswap-rewrite.c` still passes (driver changes are backward-compatible)
`check-comgr` green in CI (PSDB / compiler-ci-amd-staging)

cc @lamb-j @chinmaydd

Made with Cursor

z1-cciauto · 2026-04-22T17:13:49Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/5392

z1-cciauto · 2026-04-22T17:22:25Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/5394

chinmaydd

Generally LGTM, but @lamb-j is more familiar with testing infra.

lamb-j · 2026-04-22T20:35:52Z

+# Each LLVM backend built into the in-tree tools becomes a
+# `<target>-registered-target` feature, matching the upstream LLVM lit.cfg
+# convention. Tests that need a specific backend (e.g. the HotSwap LIT
+# harness driving clang with --offload-arch=gfx*) can gate on these rather
+# than fail noisily on builds that omit AMDGPU / X86 / etc.
+for target in config.llvm_targets_to_build.split(";"):
+    target = target.strip()
+    if target:
+        config.available_features.add(target.lower() + "-registered-target")
+


Don't think we actually have a way to exercise the case where AMDGPU target is omitted. We'd fail on the Comgr link, so probably not worth to add here.

If we drop this, the config.llvm_targets_to_build = r'@LLVM_TARGETS_TO_BUILD@' addition in lit.site.cfg.py.in and the REQUIRES: in the test can also go.

You're right. Comgr calls LLVMInitializeAMDGPU*() unconditionally so libamd_comgr.so wouldn't link without AMDGPU in the first place -- the REQUIRES: gate was defensive code that can never fire. Dropped all three pieces in 5f1e46a7:

REQUIRES: amdgpu-registered-target from hotswap-rewrite-e2e.hip

the <target>-registered-target feature loop in lit.cfg.py

the config.llvm_targets_to_build = r'@LLVM_TARGETS_TO_BUILD@' plumbing in lit.site.cfg.py.in

Net: -17 lines. I originally added it because I'd promised it in the #2203 follow-up plan, but the promise was overblown; happy to drop it here and note the reasoning inline on the #2203 thread if that makes the audit trail cleaner. If a future LIT test genuinely needs backend-gating (e.g. an x86-only stanza), we can re-add the feature machinery then.

lamb-j · 2026-04-22T20:41:10Z

[2026-04-22T18:39:19.249Z] PASS: Comgr :: hotswap-rewrite.c (8 of 25)
[2026-04-22T18:39:19.249Z] PASS: Comgr :: hotswap-rewrite-e2e.hip (12 of 25)

Cool. I do think we should probably create a hotswap/ directory to keep things organized, but we can do that later.

harsh-amd · 2026-04-22T22:46:11Z

Agreed on the hotswap/ subdirectory organization. There's enough hotswap-flavored test surface incoming (this e2e plus the opcode-level stanzas on every subsequent real-patch PR) that the split pays off quickly. I'd rather not do it in this PR since it means moving the already-landed test-lit/hotswap-rewrite.c and the test-lit/comgr-sources/hotswap-rewrite.c driver into the new location -- clean as a dedicated NFC-ish reorg PR after the first couple of patch PRs have landed and we know what the tree actually looks like. Tracked mentally as a follow-up alongside the comgr-utils extraction from #2201. cc @chinmaydd.

z1-cciauto · 2026-04-22T22:50:06Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/5404

Adds test-lit/hotswap-rewrite-e2e.hip: drives the full compile -> hotswap-rewrite -> verify chain on a real clang-produced gfx1250 code object. Uses the in-tree %clang / %llvm-readelf / %llvm-objdump / %FileCheck substitutions so the test runs out of the LLVM monorepo build without any external toolchain. Current assertions (weak patch stubs in PR ROCm#2203 keep the output bytewise-identical to the input, so we check what we can verify today): - Input is a gfx1250 ELF (e_flags carries gfx1250, AMDHSA metadata note records amdhsa.target = amdgcn-amd-amdhsa--gfx1250). - amd_comgr_hotswap_rewrite returns AMD_COMGR_STATUS_SUCCESS. - Output ELF preserves the gfx1250 identity on both channels. - .text is still present and marked PROGBITS. - llvm-objdump recognizes the output as elf64-amdgpu and disassembles at least through s_endpgm. Future per-patch PRs (in-place / trampoline / WMMA / scratch) will layer their own `llvm-objdump | FileCheck` stanzas on top of this harness to assert their specific opcode / trampoline changes. Supporting changes: - amd/comgr/test-lit/lit.cfg.py: add %llvm-readelf substitution (only %llvm-objdump was wired up previously). - amd/comgr/test-lit/comgr-sources/hotswap-rewrite.c: optional --output <path> flag to write the rewrite output to a file so LIT tests can inspect it with llvm-readelf / llvm-objdump. Existing --zero-size negative-path coverage is unchanged. Made-with: Cursor

z1-cciauto · 2026-04-23T15:37:47Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/5424

harsh-amd requested review from chinmaydd and lamb-j as code owners April 22, 2026 17:10

harsh-amd force-pushed the hotswap-lit-e2e branch 2 times, most recently from 6614457 to dfce3c4 Compare April 22, 2026 17:18

lamb-j added the hotswap Related to the Comgr Hotswap feature label Apr 22, 2026

chinmaydd approved these changes Apr 22, 2026

View reviewed changes

xintin mentioned this pull request Apr 22, 2026

[AMDGPU] comgr: HotSwap in-place patches for B0-to-A0 rewriting #2222

Merged

lamb-j reviewed Apr 22, 2026

View reviewed changes

harsh-amd force-pushed the hotswap-lit-e2e branch from dfce3c4 to 5f1e46a Compare April 22, 2026 22:45

lamb-j approved these changes Apr 22, 2026

View reviewed changes

harsh-amd force-pushed the hotswap-lit-e2e branch from 5f1e46a to 2b0a8f6 Compare April 23, 2026 15:32

yxsamliu merged commit ab27c60 into ROCm:amd-staging Apr 24, 2026
36 of 39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Comgr] Add end-to-end LIT coverage for amd_comgr_hotswap_rewrite#2291

[Comgr] Add end-to-end LIT coverage for amd_comgr_hotswap_rewrite#2291
yxsamliu merged 1 commit into
ROCm:amd-stagingfrom
harsh-amd:hotswap-lit-e2e

harsh-amd commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 22, 2026

Uh oh!

chinmaydd left a comment

Uh oh!

lamb-j Apr 22, 2026

Uh oh!

lamb-j Apr 22, 2026

Uh oh!

harsh-amd Apr 22, 2026

Uh oh!

lamb-j commented Apr 22, 2026

Uh oh!

harsh-amd commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

harsh-amd commented Apr 22, 2026

Summary

Changes

What this covers today

Future per-patch PRs

Test plan

Uh oh!

z1-cciauto commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 22, 2026

Uh oh!

chinmaydd left a comment

Choose a reason for hiding this comment

Uh oh!

lamb-j Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

lamb-j Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

harsh-amd Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

lamb-j commented Apr 22, 2026

Uh oh!

harsh-amd commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 22, 2026

Uh oh!

z1-cciauto commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants