Skip to content

[AMDGPU] comgr: Hotswap dispatcher vtable + .def registry, fix Windows support#2491

Open
xintin wants to merge 2 commits into
ROCm:amd-stagingfrom
xintin:comgr-windows-hotswap-vtable
Open

[AMDGPU] comgr: Hotswap dispatcher vtable + .def registry, fix Windows support#2491
xintin wants to merge 2 commits into
ROCm:amd-stagingfrom
xintin:comgr-windows-hotswap-vtable

Conversation

@xintin
Copy link
Copy Markdown

@xintin xintin commented May 11, 2026

Requires: to run CI in order to test the fix.

Summary

Replaces the LLVM_ATTRIBUTE_WEAK + #if !defined(_MSC_VER) patch dispatch pattern with a function-pointer vtable driven by a central comgr-hotswap-patches.def registry.

Addresses issue #2479: on Windows, LLVM_ATTRIBUTE_WEAK expands to nothing under MSVC, so the no-op stubs in comgr-hotswap-b0a0.cpp became regular definitions and either (a) collided with the strong overrides as LNK2005 duplicate symbols, or (b) silently won every dispatch when the patch .cpp files were guarded out with #if !defined(_MSC_VER), disabling hotswap entirely while still reporting build success.

The new dispatch contract is:
one slot on HotswapPatchVTable per patch family;
one alpha-ordered line in comgr-hotswap-patches.def per merged patch; one register*Patch(HotswapPatchVTable&) exported from each comgr-hotswap-patch-*.cpp;
installHotswapPatches() walks the .def to bind every slot;
retargetCodeObjectB0A0 drives the install once via std::call_once;
a missing registrar is a libamd_comgr / HotswapMCTests link error, not a silent feature disable. applyWmmaSplitPatches and applyScratchPatches keep nullptr defaults because no patch file ships them yet.
The dispatcher treats nullptr as a no-op slot, preserving the previous "weak stub returns 0" behaviour until those families land their first strong override.

Changes

  • comgr-hotswap-patches.def (new): X-macro registry with one HOTSWAP_PATCH(Name) line per merged patch (InPlace, Trampoline, Vop3px2Src2, WmmaHazard).
  • comgr-hotswap-internal.h: Add HotswapPatchVTable, getHotswapPatchVTable(), installHotswapPatches(). Forward-declare every register*Patch from the same .def so internal.h is the single prototype source for both libamd_comgr and the unit tests. Remove the per-pass applyXxx external declarations -- impls are now file-local in their patch .cpp.
  • comgr-hotswap-b0a0.cpp: Delete the six weak apply* stubs and the forward-declaration block. Add the singleton accessor and the .def-driven installHotswapPatches. Route the dispatcher through the vtable via a small runPerInstPass helper that treats nullptr slots as 0-return. Install once per process from retargetCodeObjectB0A0 under std::call_once. Liveness / DWARF stubs that still have no strong override anywhere in tree keep the LLVM_ATTRIBUTE_WEAK pattern -- they migrate the same way the first time a real implementation lands.
  • comgr-hotswap-patch-{inplace,trampoline,wmma-hazard,vop3px2-src2}.cpp: Drop the #if !defined(_MSC_VER) guard and its explanatory comment. Rename the dispatcher entry point applyXxx to a file-static applyXxxImpl. Add a file-scope registerXxxPatch(HotswapPatchVTable&) that binds the impl into the vtable. Test-facing helpers (classifyWmmaNops, patchScaleSrc2) now compile unconditionally, fixing the LNK2019 failures in HotswapMCTests on MSVC.

Tests

Link errors catch .def entries without matching register*Patch definitions. The tests cover the runtime binding behavior the linker cannot validate.

  • HotswapMCTest.cpp: 3 new TEST(HotswapPatchVTable, ...) blocks.
    • RegisterInPlaceBindsOnlyInPlaceSlot -- canonical worked example that pins the "binds only its own slot" invariant for one installer. Wrong-slot bugs in the other three register*Patch functions are caught transitively by the install end-to-end test below.
    • InstallBindsRegisteredAndLeavesUnregisteredNull -- end-to-end install: defaults null, every .def entry gets bound, slots without a .def entry (wmma-split, scratch) stay null. Pins the dispatcher's no-op contract for unimplemented pass families.
    • ProcessSingletonIdentityAndInstallPersists -- getHotswapPatchVTable() is a real Meyers singleton and bindings persist on it; the retargetCodeObjectB0A0 install path under std::call_once relies on this.
  • test-unit/CMakeLists.txt: Pull comgr-hotswap-b0a0.cpp, comgr-hotswap-patch-inplace.cpp, and comgr-hotswap-patch-trampoline.cpp into HotswapMCTests (the other two patch sources were already there) so installHotswapPatches and every register*Patch resolve at link time. A patches.def line without a matching definition produces a HotswapMCTests link error, which is itself a regression guard for the new contract.
  • All existing LIT and gtest coverage continues to apply unchanged. The dispatcher's observable Linux behaviour is preserved (same call order, same first-non-zero-wins per-instruction semantics, same whole-kernel passes after the inner loop).

Follow-up (deferred)

  • In-flight patches that still ride on the weak-symbol pattern (e.g. ds_2addr_stride64 per #2379, need a one-line .def add and a register*Patch shim when they rebase.
  • Migrate the remaining LLVM_ATTRIBUTE_WEAK liveness / DWARF stubs (buildCfg, computeLiveness, getInstRegDefUse, getBranchImm, verifyPatchCorrectness, addTrampolineSymbols, patchDebug*) onto the same .def contract the first time any of them grows a real implement.

Signed-off-by: xintin <gaurav.verma@amd.com>
@xintin xintin requested review from chinmaydd and lamb-j as code owners May 11, 2026 23:50
@xintin xintin added comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature labels May 11, 2026
Signed-off-by: xintin <gaurav.verma@amd.com>
@xintin xintin requested review from harsh-amd and martin-luecke May 12, 2026 06:45
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.

1 participant