From 44b177a3c19da0481fa845bb5d367bf5c2e6a26e Mon Sep 17 00:00:00 2001 From: Justin Card Date: Wed, 26 Aug 2026 17:25:10 -0400 Subject: [PATCH 1/3] spec(BACKEND-ROCM): commit the device-fit bounded-memory spec Issue #1934: RocmPlatform::needs_weight_staging() is stale-false, so CheckDeviceWeightFit's one call site never runs on ROCm regardless of #1870/#1935's arithmetic fix -- measured directly, a real checkpoint load with VT_DEVICE_WEIGHT_BUDGET_BYTES=1 produced no refusal. Records why flipping needs_weight_staging() itself was rejected (IndexedGdnStateIoEnabled already checks op registration directly and would regress; several sibling GDN consumers have no such fallback and would activate unverified kernels) in favor of a new, narrower Platform predicate before any implementation lands. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code] --- .../specs/rocm-device-fit-bounded-memory.md | 238 ++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 .agents/specs/rocm-device-fit-bounded-memory.md diff --git a/.agents/specs/rocm-device-fit-bounded-memory.md b/.agents/specs/rocm-device-fit-bounded-memory.md new file mode 100644 index 000000000..c315d02fc --- /dev/null +++ b/.agents/specs/rocm-device-fit-bounded-memory.md @@ -0,0 +1,238 @@ +# BACKEND-ROCM-DEVICE-FIT-BOUNDED-MEMORY — make the #1123/#1870 refusal reachable on ROCm, without moving GDN kernel defaults + +Issue: [#1934](https://github.com/mudler/vllm.cpp/issues/1934). +Related: [#1870](https://github.com/mudler/vllm.cpp/issues/1870) (stays open; +this row is its actual remaining blocker), [#1928](https://github.com/mudler/vllm.cpp/issues/1928) +(unrelated ROCm gap, not touched here). +Base: `2a42cb369` (`upstream/main` at the claim). + +## Scope + +`CheckDeviceWeightFit`'s one production call site (`model_loader.cpp`) reads +`target.needs_weight_staging()` to decide whether the load-time device-fit +refusal (#1123, and the arithmetic fix #1870/#1935 made to it) runs at all. +`RocmPlatform::needs_weight_staging()` hardcodes `false` — a W0-era comment +says a discrete AMD card "will eventually answer true... Revisit at M2", never +revisited despite ROCm landing grouped-GEMM, MoE combine/gate, ROCM_ATTN and +hipGraph decode capture since. Measured directly on this box (gfx1200): +`VT_DEVICE_WEIGHT_BUDGET_BYTES=1` against a real checkpoint produced no +refusal at all, because `CheckDeviceWeightFit` returns before computing +anything when its `needs_weight_staging` argument is false. Meanwhile the +actual device allocation the refusal exists to guard — `d.b.Alloc(nb)` in +`qwen3_5.cpp`'s `ResidentWeight` — is NOT gated by that flag; it runs +unconditionally on any non-CPU platform (since issue #125's fix keyed the +alias-vs-upload branch on `is_cpu()`, not `needs_weight_staging()`). So #1870's +`hipMalloc: out of memory` stays reachable regardless of the arithmetic fix. + +IN SCOPE: + +- Make the device-fit refusal reachable on ROCm: a real, once-probed + `ResidencyPolicy::device_memory_total_bytes` and a way for the ONE call site + to know a budget check applies here. +- Do it WITHOUT flipping `needs_weight_staging()` itself, because that flag is + documented (`interface.h`) to also gate the indexed/merged/packed GDN + kernel-dispatch defaults, at least one of which (`MergedGdnBaEnabled` / + `MergedGdnQkvzEnabled` / the packed-decode and fp8-resident-prep gates) has + NO op-registration fallback — it would newly activate unconditionally on + ROCm the moment the flag moves, exactly the "assume kernels a device might + not have" failure `IndexedGdnStateIoEnabled` was written to avoid for the + ONE consumer that already checks op registration. Auditing and (where + needed) porting each of those kernel paths for ROCm correctness is a + separate, larger undertaking than this row. + +OUT OF SCOPE: + +- `needs_weight_staging()` itself, and every one of its other consumers + (`DirectDeviceLoadEligible`, `IndexedGdnStateIoEnabled`, `MergedGdnBaEnabled`, + `MergedGdnQkvzEnabled`, `PackedGdnDecodeEligibility`'s eligibility struct, + `PrepareGdnFp8Resident`, `PrepareBf16Resident`). None of their behavior moves + in this row, and the tests prove it. +- `ResidencyPolicy::release_host_weights_after_upload` and + `uses_device_memory_pool` for ROCm. CUDA sets both `true` for its own + Marlin/DevicePool reasons; this row leaves ROCm's answer at the existing + default (`false`) because those are separate, unmeasured policy questions, + not a side effect of making the budget check reachable. +- The `kMoeGroupedGemmBf16` ROCm provider gap (#1928). Unrelated to this row. + +## Upstream chain + +No upstream vLLM mirror for the new predicate — `Platform::needs_weight_staging()` +itself has none (it is a vllm.cpp-original policy seam, per its own doc), and +this row adds a narrower sibling to it for the same reason. The +`device_memory_total_bytes` probe mirrors `platforms/cuda.cpp`'s own +`cudaMemGetInfo` call at registration, HIP's exact analog. + +## Design + +**A new `Platform` method, not a flipped flag.** `Platform::allocates_bounded_device_memory()` +(`interface.h`) answers the narrow physical question `CheckDeviceWeightFit` +actually needs: does a load here draw from a bounded, budget-checkable device +memory pool? Default implementation delegates to `needs_weight_staging()` — +byte-identical for CUDA (already true) and every platform that overrides +neither method (still false). `RocmPlatform` overrides ONLY this new method to +`true`, leaving `needs_weight_staging()` at its existing `false`. + +`model_loader.cpp`'s ONE `CheckDeviceWeightFit` call site reads +`target.allocates_bounded_device_memory()` instead of +`target.needs_weight_staging()`. The OTHER `needs_weight_staging()` read in the +same function (the streamed-expert-lane condition, a few lines above) is +UNTOUCHED — that one genuinely asks "is the fully-optimized device-resident +forward what's running", which `needs_weight_staging()` still correctly +answers. + +**Why not just flip `needs_weight_staging()`.** Read literally: +`IndexedGdnStateIoEnabled` (`qwen3_5.cpp`) ALREADY takes ROCm's fast arm today, +independent of `needs_weight_staging()` — its `!needs_weight_staging()` branch +falls through to `IndexedGdnOpsNative(device)` for any non-CPU platform, +checking `kCausalConv1dUpdate`/`kGdnDecode`/`kGdnStateGather`/`kGdnStateScatter` +registration directly (all four ARE registered for ROCm — confirmed via +`rocm_ops.hip`). Flipping `needs_weight_staging()` would make this consumer +take the OTHER branch (`return indexed == nullptr || indexed[0] != '0';`), +which DROPS the op-registration check and assumes the ops exist — a +regression, not an improvement, for this one consumer. `MergedGdnBaEnabled` +and its siblings have no such fallback at all: they are a bare +`enabled && needs_weight_staging()`, so flipping the flag activates them on +ROCm unconditionally, and whether the underlying merged/packed GEMM and +fp8-resident-prep paths are numerically correct there is unverified. A new, +narrow predicate sidesteps all of it. + +**The probe.** `RocmPlatform` (`rocm.cpp`) is deliberately HIP-header-free — +compiled as a stub object even in non-HIP builds, reaching the device only +through the `vt::rocm::` free-function seam (`rocm_runtime.h`), the same +discipline `DeviceAvailable()`/`HostMemoryIsDeviceAddressable()` already use. +A NEW free function, `vt::rocm::DeviceMemoryTotalBytes(int index)`, declared +there and implemented in `rocm_backend.hip` (`hipSetDevice` + `hipMemGetInfo`, +mirroring `Backend::DeviceMemoryInfo`'s existing body without calling it — +that method is a LIVE per-request probe with its own single consumer, +`Gemma4MoE`'s expert LRU, and reusing it for `ResidencyPolicy` would be the +exact "wake a currently-dead-on-CUDA seam" scope-widening `interface.h`'s own +comment on `device_memory_total_bytes` names as issue #1126's territory — a +DIFFERENT platform's DIFFERENT concern, not this row's). The platform +registrar calls it ONCE, device 0, and threads the result through a new +`RocmPlatform` constructor parameter into a stored member, exactly mirroring +`CudaPlatform`'s own constructor-injected `device_memory_total_bytes_`. + +`RocmPlatform::residency_policy()` returns `{device_memory_total_bytes = }` +with every other field at its existing default (`false`/`0`) — see Out of +Scope. + +## Risks and decisions + +- **The new predicate's name.** `allocates_bounded_device_memory()` was chosen + over reusing `needs_weight_staging` under a different value, or deriving the + gate implicitly from `residency_policy().device_memory_total_bytes != 0` + alone. An explicit, documented predicate matches this codebase's own + established style (`is_integrated_gpu`/`is_unified_memory`/ + `host_memory_is_device_addressable`/`needs_weight_staging` are all + deliberately separate, cross-referenced concepts with a paragraph each + explaining why they are not one another) rather than an implicit + budget-nonzero proxy a future reader would have to reverse-engineer. +- **Two predicates that look related but move independently is exactly the + shape issue #125 already burned this codebase on once** + (`needs_weight_staging()` vs. `is_cpu()` for `ResidentWeight`'s alias-vs-upload + branch). This row adds a THIRD closely-related predicate rather than fixing + the second one's scope, which could read as compounding the confusion. Argued + against a merge: `needs_weight_staging()` and `allocates_bounded_device_memory()` + answer genuinely different questions (device-resident FORWARD KERNELS vs. a + BUDGET GUARD on ordinary weight upload) that happen to coincide on CUDA and + deliberately diverge on ROCm — the tests pin the divergence rather than + asserting it away. +- **`vt::rocm::DeviceMemoryTotalBytes` vs. `Backend::DeviceMemoryInfo`.** + Considered reusing the existing backend method directly from the platform + registrar via `vt::GetBackend(DeviceType::kROCM).DeviceMemoryInfo(...)` and + rejected: static-init order across translation units is unspecified (the + registrar's own comment states this as the reason it probes the device + itself rather than trusting `RocmBackend`'s registrar to have already run), + so a call through `GetBackend` at platform-registration time risks reading + an unregistered backend. The free-function seam sidesteps the ordering + question entirely, the same way `DeviceAvailable()` already does. +- **CPU cost of a `hipSetDevice`+`hipMemGetInfo` call at every process start + on a ROCm build.** Bounded: it runs exactly once, in the platform registrar, + which already makes comparable device-attribute probe calls + (`ProbeDevice`'s own `hipGetDeviceProperties`-class calls) at the same point. + +## Tests + +**`tests/vllm/platforms/test_platform.cpp`**: + +- CPU and the existing `FakeCapabilityPlatform` (`sm121`) cases gain a + `CHECK(x.allocates_bounded_device_memory() == x.needs_weight_staging())` + pin, proving the default delegation is a true no-op for every platform that + does not override the new method — red before this row (the method does not + exist), trivially green after. +- A new fake platform, `FakeBoundedNonStagingPlatform`, constructing ROCm's + own real shape (`needs_weight_staging()=false`, + `allocates_bounded_device_memory()=true`) on the CPU tier — proves the two + predicates are independently settable, mirroring the existing + `FakeUnifiedAddressablePlatform` pattern this file already uses for + `host_memory_is_device_addressable()` vs. its neighbours. +- A new HIP-build-only case, skipped via `if (!HasPlatform(DeviceType::kROCM)) return;` + (matching the existing CUDA-only case's pattern), asserting on THIS box's + real `RocmPlatform`: `needs_weight_staging()` still false, + `allocates_bounded_device_memory()` true, `residency_policy().device_memory_total_bytes > 0` + (the probe actually ran), and the two policy fields this row deliberately + left alone still read false. + +**`tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp`**: `StagingPlatform` +gains two independently-settable flags +(`needs_weight_staging_flag`, `allocates_bounded_device_memory_flag`, +previously the class hardcoded `needs_weight_staging()` to `true` and had no +override for the new method). Two new cases: one with +`(false, true)` — ROCm's exact real state — proving the refusal STILL fires; +one with `(true, false)` proving it does NOT (the call site reads the new +predicate, not the old one, in both directions). Every EXISTING case in this +file is unaffected: the default-constructed flags (`true`, `true`) reproduce +today's behavior byte for byte because the new method's default delegates to +the old one, which the class already overrides to `true`. + +## Gates + +- `./build-hip/tests/test_platform` (ROCm build, this box, gfx1200) — the + HIP-only case above is the one that can only run here +- `./build-hip/tests/test_gguf_device_fit_reach` +- `./build-nix-cpu/tests/test_platform` / `test_gguf_device_fit_reach` (CPU + build, same box) — every other case +- A real hardware repro: with this row, `VT_DEVICE_WEIGHT_BUDGET_BYTES=1` + against a real checkpoint on this box's gfx1200 card refuses (it did not, + before this row) — the exact measurement #1934 reports as missing. +- `scripts/agent-preflight.sh --staged` (against the true fork point, not + `origin/main` — see `.agents/developer-preferences.md`) + +## Evidence + +Recorded in the pull request body: red-before/green-after for both test +files, and the real-hardware before/after transcript +(`VT_DEVICE_WEIGHT_BUDGET_BYTES=1` refusing where it previously did not). + +## Owed + +- **The GDN kernel-dispatch audit.** `needs_weight_staging()` itself stays + false on ROCm, so `DirectDeviceLoadEligible`, `IndexedGdnStateIoEnabled` + (already independently safe — see Design), `MergedGdnBaEnabled`, + `MergedGdnQkvzEnabled`, the packed-decode eligibility struct, and the fp8/bf16 + GDN resident-prep gates all keep their current ROCm behavior. Whether ROCm + SHOULD eventually take the optimized arm for the ones with no op-registration + fallback is a real, separate question this row does not answer — it needs a + per-consumer op-registration and token-exact audit, not a flag flip. No + issue filed yet for this specific follow-up beyond the standing note in + #1934 itself; file one before starting that audit. +- **`release_host_weights_after_upload` / `uses_device_memory_pool` for ROCm.** + Left at `false`/default deliberately (see Out of Scope); a discrete ROCm + card may want different values once measured, same as the CUDA leg's own + comment describes for its choices. + +## Stop conditions + +Stop and report rather than widening scope if closing the "Owed" GDN audit +turns out to be a precondition for something THIS row needs — it should not +be: every existing GDN dispatch path is provably unmoved by this row's tests. + +Stop rather than reusing `Backend::DeviceMemoryInfo` directly from the +platform registrar (see Risks) even if it looks like less code — the +static-init-order hazard is real and already named as the reason the +registrar probes the device itself elsewhere in this same file. + +## Now + +`ACTIVE`. Spec committed; implementation, tests and the real-hardware repro +follow in this same pull request. From 0c97cfb914ec2c8cec94adf7359157738b3d8625 Mon Sep 17 00:00:00 2001 From: Justin Card Date: Wed, 26 Aug 2026 17:56:09 -0400 Subject: [PATCH 2/3] fix(BACKEND-ROCM): a narrow predicate makes the device-fit refusal reachable on ROCm Issue #1934. CheckDeviceWeightFit's one call site read target.needs_weight_staging(), which RocmPlatform hardcodes false -- a stale W0-era placeholder never revisited despite ROCm landing grouped-GEMM, MoE combine/gate, ROCM_ATTN and hipGraph decode capture since. Measured directly: VT_DEVICE_WEIGHT_BUDGET_BYTES=1 against a real checkpoint produced no refusal at all, while the actual device allocation the refusal guards (ResidentWeight's d.b.Alloc) runs unconditionally on any non-CPU platform since issue #125's is_cpu() fix -- so #1870's crash stayed reachable regardless of that row's arithmetic fix. Rejected flipping needs_weight_staging() itself: that flag also gates several GDN kernel-dispatch defaults (merged/packed projections, fp8/bf16 resident prep) with no op-registration fallback, which would newly activate unconditionally on ROCm the moment it moved -- IndexedGdnStateIoEnabled already takes ROCm's fast arm today by checking op registration directly rather than trusting this flag, and flipping it would remove that check for that one consumer while blindly trusting the others. Adds Platform::allocates_bounded_device_memory(), default-delegating to needs_weight_staging() (byte-identical for every platform that overrides neither), RocmPlatform overrides only this new method to true. residency_policy() now carries a real, once-probed device_memory_total_bytes via a new HIP-free vt::rocm::DeviceMemoryTotalBytes free function (mirrors DeviceAvailable()'s registrar-independence reasoning; not Backend::DeviceMemoryInfo, whose live per-request probe is a different, currently-CUDA-dead seam per issue #1126). The other two ResidencyPolicy fields stay at their existing default. model_loader.cpp's ONE CheckDeviceWeightFit call site now reads the new predicate; the other needs_weight_staging() read in the same function (the streamed-expert-lane condition) is untouched. Red-before: every new test references the method before it exists, captured by stashing the implementation and rebuilding (test_platform and test_gguf_device_fit_reach both fail to compile). Green-after: both pass in full, including new cases proving the two predicates move independently in both directions and that every existing GDN kernel-dispatch consumer of needs_weight_staging() is unmoved. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code] --- include/vllm/platforms/interface.h | 41 +++++++++ include/vt/rocm/rocm_runtime.h | 14 +++ src/vllm/entrypoints/model_loader.cpp | 11 ++- src/vllm/platforms/rocm.cpp | 73 ++++++++++++---- src/vt/rocm/rocm_backend.hip | 11 +++ .../test_gguf_device_fit_reach.cpp | 73 +++++++++++++++- tests/vllm/platforms/test_platform.cpp | 86 +++++++++++++++++++ 7 files changed, 291 insertions(+), 18 deletions(-) diff --git a/include/vllm/platforms/interface.h b/include/vllm/platforms/interface.h index 1955c2c4c..9c5a2e727 100644 --- a/include/vllm/platforms/interface.h +++ b/include/vllm/platforms/interface.h @@ -372,6 +372,47 @@ class Platform { // memory in place) — the decoupling a bare `kCUDA` cannot express. virtual bool needs_weight_staging() const { return false; } + // BACKEND-ROCM, issue #1934. Does a load on this platform allocate BOUNDED, + // budget-checkable device memory for resident weights — the narrower + // PHYSICAL question `needs_weight_staging()` deliberately does NOT answer. + // + // `needs_weight_staging()`'s own doc above says it "governs the model's + // device-resident forward as a whole", listing `ResidentWeight` among what it + // gates — that listing is STALE. `qwen3_5.cpp::ResidentWeight` was fixed + // under issue #125 to key its alias-vs-upload branch on `is_cpu()`, not + // `needs_weight_staging()`, precisely because the old predicate answered + // false for every non-CUDA device (Vulkan, Metal, XPU, ROCm) and each of + // them aliased a HOST pointer into a DEVICE kernel. So today, on ANY + // non-CPU platform, `ResidentWeight::Alloc` genuinely allocates bounded + // device memory regardless of `needs_weight_staging()` — confirmed on ROCm + // by issue #1870's own reproduction, a real `hipMalloc: out of memory`. + // + // What `needs_weight_staging()` correctly still gates is a DIFFERENT + // question: should the OPTIMIZED device-resident forward run — the indexed + // GDN state-I/O kernels (though `IndexedGdnStateIoEnabled` already + // special-cases a non-staging, non-CPU device by checking op registration + // directly, so ROCm already takes the fast arm there without this method), + // the merged/packed GDN projections, and the fp8/bf16 GDN resident-prep + // passes. Those default to the ROW-COPY REFERENCE path today on ROCm, and + // this method changes NONE of them: flipping `needs_weight_staging()` + // itself was considered and rejected (see the row's spec) because at least + // one of those consumers has no op-registration fallback and would silently + // assume kernels a device might not have, exactly the failure mode + // `IndexedGdnStateIoEnabled` was written to avoid for the ONE consumer that + // already checks. + // + // Consumed by the ONE production call site of `CheckDeviceWeightFit` + // (`gguf_device_fit.h`, issue #1123): the load-time refusal only needs to + // know "will this load draw from a bounded device memory pool, and do we + // know its size" — not "should the fully-optimized forward run". + // + // Default DELEGATES to `needs_weight_staging()`, so CUDA's answer (true) is + // unchanged and every platform that overrides neither method reads exactly + // as it did before this method existed — a pure additive seam. + virtual bool allocates_bounded_device_memory() const { + return needs_weight_staging(); + } + // Does this platform have the fused flash-attention-2 (native-bf16) attention // fast path? The FA2 dispatch (qwen3_5.cpp GdnBlockPaged / full-attn preamble) // emits bf16 q/k and a bf16 attention output — the combo the vendored CUDA diff --git a/include/vt/rocm/rocm_runtime.h b/include/vt/rocm/rocm_runtime.h index d51d55f64..9f14b7620 100644 --- a/include/vt/rocm/rocm_runtime.h +++ b/include/vt/rocm/rocm_runtime.h @@ -10,6 +10,7 @@ // at load time on a machine that merely happens to have HIP installed. #pragma once +#include #include namespace vt::rocm { @@ -55,4 +56,17 @@ bool HostMemoryIsDeviceAddressable(int index) noexcept; // path their silicon took without reading driver internals. bool ManagedAllocActive(int index) noexcept; +// BACKEND-ROCM, issue #1934. `hipMemGetInfo`'s `total` for device `index`, in +// bytes; 0 when the device is absent or the probe fails. HIP-free so the +// PLATFORM registrar (static-init time, unspecified cross-TU order — the same +// reasoning `DeviceAvailable()` above states) can read it without depending on +// `RocmBackend`'s own registrar having already run. Mirrors +// `platforms/cuda.cpp`'s own `cudaMemGetInfo` probe at registration, which +// this project's ResidencyPolicy::device_memory_total_bytes doc already +// specifies as "TOTAL rather than FREE, because free at load time carries the +// page cache and whatever else the box is doing" — same reasoning applies to +// HIP's allocator. 0 == UNKNOWN, which `gguf_device_fit.h`'s load-time +// refusal already reads as "do not decide", never as "nothing fits". +size_t DeviceMemoryTotalBytes(int index) noexcept; + } // namespace vt::rocm diff --git a/src/vllm/entrypoints/model_loader.cpp b/src/vllm/entrypoints/model_loader.cpp index f8cb0793e..3c4e3f412 100644 --- a/src/vllm/entrypoints/model_loader.cpp +++ b/src/vllm/entrypoints/model_loader.cpp @@ -2402,9 +2402,18 @@ std::unique_ptr LoadedEngine::FromModelDir( // already excludes every load it could apply to. const bool policy_forces_full_expand = GgufPolicyForcesFullExpand(gguf_load_policy); + // BACKEND-ROCM (#1934): `allocates_bounded_device_memory()`, not + // `needs_weight_staging()`. The two questions differ (see the interface + // doc): this one asks whether `ResidentWeight` draws from a bounded + // device pool at all -- true on every non-CPU platform since issue + // #125's `is_cpu()` fix -- while `needs_weight_staging()` asks whether + // the FULLY-OPTIMIZED device-resident forward (several GDN kernel + // defaults) should run. Using the narrower predicate here is what makes + // this refusal reachable on ROCm without moving any of the other one's + // consumers; the row's spec records why that flag stays untouched. const DeviceWeightFit fit = CheckDeviceWeightFit( gguf, vt::DeviceTypeName(target.device_type()), - target.needs_weight_staging(), + target.allocates_bounded_device_memory(), DeviceWeightBudgetBytes( target.residency_policy().device_memory_total_bytes), /*model_dtype_bytes=*/2, lane, policy_forces_full_expand); diff --git a/src/vllm/platforms/rocm.cpp b/src/vllm/platforms/rocm.cpp index dca5b962f..6567f7be2 100644 --- a/src/vllm/platforms/rocm.cpp +++ b/src/vllm/platforms/rocm.cpp @@ -18,6 +18,7 @@ // value here is a guess dressed as a decision. #include "vllm/platforms/interface.h" +#include #include #include "vt/backend.h" @@ -28,6 +29,15 @@ namespace { class RocmPlatform final : public Platform { public: + // Issue #1934. `device_memory_total_bytes` is `vt::rocm::DeviceMemoryTotalBytes(0)`, + // probed once by the registrar below at static init, exactly mirroring how + // `CudaPlatform` threads its own `cudaMemGetInfo` probe through its + // constructor (`platforms/cuda.cpp`). 0 means the probe failed or no device + // is present; `residency_policy()` passes it through unexamined, and + // `gguf_device_fit.h` already reads 0 as UNKNOWN rather than "nothing fits". + explicit RocmPlatform(size_t device_memory_total_bytes) + : device_memory_total_bytes_(device_memory_total_bytes) {} + DeviceType device_type() const override { return DeviceType::kROCM; } Backend& backend() const override { return vt::GetBackend(DeviceType::kROCM); } @@ -86,24 +96,46 @@ class RocmPlatform final : public Platform { // snapshot. This flag still stays false because flipping it to engage a // real model's decode-graph path is W2, not W1. // needs_weight_staging() stays false: this is the memory-model POLICY that - // selects the device-resident forward over the host-resident reference path. - // HIP's programming model does stage (hipMalloc hands back a distinct - // address), so a discrete AMD card will eventually answer true — but in W0 - // there is one registered op, so the only path that can run at all is the - // host-resident one the reference tier serves on a unified part. Answering - // true today would route a model into a path with no kernels. Revisit at M2. + // selects the FULLY-OPTIMIZED device-resident forward (indexed GDN state + // I/O with no op-registration fallback for a couple of its consumers, + // merged/packed GDN projections, fp8/bf16 GDN resident prep) over the + // host-resident reference path for THOSE specific kernels. Issue #1934 + // measured that several of those consumers have no per-op fallback and + // would silently assume kernels this device might not register, so + // flipping this blindly was rejected — see that issue and + // `allocates_bounded_device_memory()` below, which answers the NARROWER + // question the load-time device-fit check actually needs without moving + // any of this flag's other consumers. `IndexedGdnStateIoEnabled` already + // takes ROCm's fast arm today regardless of this flag, by checking op + // registration directly rather than trusting this policy bit — the same + // move #1934 makes for the device-fit check. // supports_fa2_attention() / opaque_attention_op() stay false: no ROCm // attention kernel exists yet. See get_attn_backend_priority below. - // The residency/memory-model policy. DEFAULTS in W0, and the reason is not - // laziness: on a unified part (780M, Strix Halo) freeing the host copy after - // "upload" would free the ONLY copy, which is the same answer the CPU, Metal - // and Vulkan platforms give for the same reason. A DISCRETE card genuinely - // wants release_host_weights_after_upload=true and a pooled allocator, and - // that is a per-DEVICE answer this per-DEVICE-TYPE seam cannot express yet - // (the CUDA leg has the same shape and has not needed to). Flip it when a - // discrete board actually loads weights, with the measurement in the record. - ResidencyPolicy residency_policy() const override { return {}; } + // BACKEND-ROCM, issue #1934. The ONE narrow question the load-time + // device-fit refusal (`gguf_device_fit.h`, issue #1123) needs answered: + // does a load here allocate device memory `ResidentWeight` cannot exceed + // unnoticed? Yes, unconditionally, on any non-CPU platform since issue + // #125's fix (see the interface doc on this method) — independent of + // `needs_weight_staging()`, which this method deliberately does not touch. + bool allocates_bounded_device_memory() const override { return true; } + + // The residency/memory-model policy. `device_memory_total_bytes` is now a + // REAL probe (issue #1934, mirrors `CudaPlatform`'s own `cudaMemGetInfo` + // probe) — the ONE field the device-fit check reads. The other two fields + // stay DEFAULT/false, unlike CUDA's: `release_host_weights_after_upload` + // and `uses_device_memory_pool` are separate policy questions (a discrete + // card's host-copy release and DevicePool reuse) this row does not touch, + // because on a unified part (780M, Strix Halo) freeing the host copy after + // "upload" would free the ONLY copy — the same answer CPU, Metal and Vulkan + // give for the same reason, and per-DEVICE (not per-DEVICE-TYPE) besides. + // Flip those when a discrete board's release/pool behavior is actually + // measured, not as a side effect of making the budget check reachable. + ResidencyPolicy residency_policy() const override { + ResidencyPolicy p; + p.device_memory_total_bytes = device_memory_total_bytes_; + return p; + } // Attention-backend priority — M3 (issue #41). Mirrors rocm.py:407-441 // `_get_backend_priorities` at pin 555967922. The dense branch is @@ -130,6 +162,9 @@ class RocmPlatform final : public Platform { return {"ROCM_ATTN", "ROCM_AITER_FA", "ROCM_AITER_UNIFIED_ATTN", "TRITON_ATTN", "TURBOQUANT"}; } + + private: + size_t device_memory_total_bytes_ = 0; }; // Registers kROCM during static init (registration completes before main() per @@ -143,7 +178,13 @@ class RocmPlatform final : public Platform { struct Registrar { Registrar() noexcept { if (!vt::rocm::DeviceAvailable()) return; - static RocmPlatform platform; + // Issue #1934. Device 0, matching this leg's other single-device probes + // (`host_memory_is_device_addressable()` above states the same choice). + // HIP-free free function, not `Backend::DeviceMemoryInfo`: the backend's + // OWN registrar (`rocm_backend.hip`) may not have run yet at this point — + // static-init order across TUs is unspecified, the same reason this + // registrar probes the device itself rather than trusting one. + static RocmPlatform platform(vt::rocm::DeviceMemoryTotalBytes(0)); RegisterPlatform(DeviceType::kROCM, &platform); } } registrar; diff --git a/src/vt/rocm/rocm_backend.hip b/src/vt/rocm/rocm_backend.hip index 121aacf6d..e09f3b292 100644 --- a/src/vt/rocm/rocm_backend.hip +++ b/src/vt/rocm/rocm_backend.hip @@ -460,4 +460,15 @@ bool ManagedAllocActive(int index) noexcept { return caps.valid && UseManagedAlloc(caps); } +size_t DeviceMemoryTotalBytes(int index) noexcept { + // Mirrors `Backend::DeviceMemoryInfo` (above) byte-for-byte, deliberately + // NOT calling it: this is a HIP-free, backend-registration-independent + // free function meant for the platform registrar (see the declaration), + // where `GetBackend(kROCM)` may not have run yet. + if (hipSetDevice(index) != hipSuccess) return 0; + size_t free_b = 0, tot_b = 0; + if (hipMemGetInfo(&free_b, &tot_b) != hipSuccess) return 0; + return tot_b; +} + } // namespace vt::rocm diff --git a/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp b/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp index e22d8b807..8337cad18 100644 --- a/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp +++ b/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp @@ -100,7 +100,16 @@ class StagingPlatform final : public vllm::platforms::Platform { std::vector supported_dtypes() const override { return {vt::DType::kBF16}; } - bool needs_weight_staging() const override { return true; } + bool needs_weight_staging() const override { return needs_weight_staging_flag; } + // BACKEND-ROCM (#1934). Overridden explicitly, NOT left to the default + // delegation to `needs_weight_staging()`, and independently settable: the + // two are proven-independent inputs to `CheckDeviceWeightFit`'s call site + // by a case that sets them to DIFFERENT values, mirroring ROCm's own real + // production state (`needs_weight_staging()=false`, + // `allocates_bounded_device_memory()=true`). + bool allocates_bounded_device_memory() const override { + return allocates_bounded_device_memory_flag; + } // ENG-EXPERT-STREAM-DEVICE W0d (#1124). The second half of the loader's lane // condition. A settable field for the same reason `create_queue_throws` is // one: the platform registry is process-global, so a second registration would @@ -115,6 +124,8 @@ class StagingPlatform final : public vllm::platforms::Platform { } bool host_addressable = false; + bool needs_weight_staging_flag = true; + bool allocates_bounded_device_memory_flag = true; private: HostBackend& backend_; @@ -882,3 +893,63 @@ TEST_CASE("device fit: the VARIABLE beats the config key, through the loader") { CHECK(message.find("cannot serve this GGUF") != std::string::npos); CHECK(message.find(std::to_string(kStagedLowerBound - 1)) != std::string::npos); } + +// --- BACKEND-ROCM (#1934): the refusal is gated on -------------------------- +// --- `allocates_bounded_device_memory()`, never on `needs_weight_staging()` - +// +// Issue #1934: `RocmPlatform::needs_weight_staging()` is stale-false, so +// before this row the ONE production call site of `CheckDeviceWeightFit` +// never ran on ROCm regardless of budget. The fix is `model_loader.cpp` +// reading `target.allocates_bounded_device_memory()` instead. These two cases +// pin that the call site reads the NEW predicate and NOT the old one, in both +// directions, so a regression that reverted the call site to +// `needs_weight_staging()` — or one that read a `||` instead of the plain +// predicate — goes red here. + +TEST_CASE( + "device fit: ROCm's own state (staging=false, bounded-memory=true) " + "still refuses") { + RegisterFakeStagingPlatform(); + Platform().needs_weight_staging_flag = false; + Platform().allocates_bounded_device_memory_flag = true; + TempFile f(BuildSyntheticMoeGguf()); + + vllm_test::SetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES", + std::to_string(kStagedLowerBound - 1)); + const std::string message = ThrownMessage(f.path(), vllm::Device::kNamedPlatform); + vllm_test::UnsetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES"); + Platform().needs_weight_staging_flag = true; + Platform().allocates_bounded_device_memory_flag = true; + + REQUIRE_FALSE(message.empty()); + CAPTURE(message); + CHECK(message.find("cannot serve this GGUF") != std::string::npos); + CHECK(message.find(std::to_string(kStagedLowerBound)) != std::string::npos); + CHECK(message.find(std::to_string(kStagedLowerBound - 1)) != std::string::npos); + CHECK(message.find("tokenizer") == std::string::npos); +} + +TEST_CASE( + "device fit: a platform that stages but reports no bounded memory is " + "NEVER refused") { + RegisterFakeStagingPlatform(); + Platform().needs_weight_staging_flag = true; + Platform().allocates_bounded_device_memory_flag = false; + TempFile f(BuildSyntheticMoeGguf()); + + // One byte under the footprint, exactly the budget the positive-control case + // above refuses at. The ONLY thing that moved is + // `allocates_bounded_device_memory_flag`. + vllm_test::SetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES", + std::to_string(kStagedLowerBound - 1)); + const std::string message = ThrownMessage(f.path(), vllm::Device::kNamedPlatform); + vllm_test::UnsetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES"); + Platform().allocates_bounded_device_memory_flag = true; + + REQUIRE_FALSE(message.empty()); + CAPTURE(message); + CHECK(message.find("cannot serve this GGUF") == std::string::npos); + // The LATER error, asserted positively: without it, "no refusal" would also + // be true of a load that died earlier for an unrelated reason. + CHECK(message.find("tokenizer: GGUF missing kv") != std::string::npos); +} diff --git a/tests/vllm/platforms/test_platform.cpp b/tests/vllm/platforms/test_platform.cpp index 90cc8ba4a..e1a02e42f 100644 --- a/tests/vllm/platforms/test_platform.cpp +++ b/tests/vllm/platforms/test_platform.cpp @@ -69,6 +69,12 @@ TEST_CASE("CPU platform is self-registered and advertises CPU capabilities") { // in place (no staging) and has no FA2 kernel, exactly what the converted // `device==kCUDA` gates answered on a CPU device (byte-identical). CHECK_FALSE(cpu.needs_weight_staging()); + // BACKEND-ROCM (#1934). The CPU leg overrides neither method, so the new + // predicate's default delegation to needs_weight_staging() is what answers + // here — proving the default is a true no-op for a platform that has not + // opted into the narrower question. + CHECK_FALSE(cpu.allocates_bounded_device_memory()); + CHECK(cpu.allocates_bounded_device_memory() == cpu.needs_weight_staging()); CHECK_FALSE(cpu.supports_fa2_attention()); // Proof that needs_weight_staging() is NOT is_unified_memory() in disguise: CPU // is UNIFIED (host==device memory) yet does NOT stage. The two predicates @@ -325,6 +331,11 @@ TEST_CASE("is_device_capability_family matches any .x (interface.py:481-4 CHECK_FALSE(sm121.is_cpu()); CHECK_FALSE(sm121.needs_weight_staging()); CHECK(sm121.is_cpu() == sm121.needs_weight_staging()); + // BACKEND-ROCM (#1934). Same base-default proof as needs_weight_staging() + // itself: this stub overrides neither method, so the delegation is what + // answers, and it answers false here exactly as needs_weight_staging() does. + CHECK_FALSE(sm121.allocates_bounded_device_memory()); + CHECK(sm121.allocates_bounded_device_memory() == sm121.needs_weight_staging()); CHECK_FALSE(sm121.opaque_attention_op()); CHECK_FALSE(sm121.support_static_graph_mode()); @@ -398,6 +409,81 @@ TEST_CASE("host_memory_is_device_addressable is independent of every neighbour") gb10.backend().DeviceMemoryIsHostAddressable()); } +namespace { +// BACKEND-ROCM, issue #1934. ROCm's own real shape, constructible on a CPU +// tier because `RocmPlatform` only compiles into a HIP build: a platform that +// does NOT need the fully-optimized device-resident forward +// (`needs_weight_staging() == false`, so none of that flag's other GDN +// kernel-dispatch consumers move) but DOES allocate bounded, budget-checkable +// device memory for resident weights (`allocates_bounded_device_memory() == +// true`, so the load-time GGUF fit refusal runs). If the two predicates were +// secretly the same bit, this platform could not exist. +class FakeBoundedNonStagingPlatform final : public Platform { + public: + DeviceType device_type() const override { return DeviceType::kROCM; } + vt::Backend& backend() const override { return vt::GetBackend(DeviceType::kCPU); } + DeviceCapability get_device_capability() const override { return {12, 0}; } + std::vector supported_dtypes() const override { return {DType::kBF16}; } + ResidencyPolicy residency_policy() const override { + ResidencyPolicy p; + p.device_memory_total_bytes = 17095983104; // this box's real gfx1200 probe + return p; + } + bool needs_weight_staging() const override { return false; } + bool allocates_bounded_device_memory() const override { return true; } +}; +} // namespace + +TEST_CASE("allocates_bounded_device_memory is independent of needs_weight_staging") { + FakeBoundedNonStagingPlatform rocm_like; + + // The value threads through the virtual at all: a platform that overrides it + // answers its own value, not the delegated default. + CHECK(rocm_like.allocates_bounded_device_memory()); + + // AGAINST needs_weight_staging, in ROCm's own real direction (issue #1934): + // FALSE there, TRUE here. A load-time refusal keyed on needs_weight_staging() + // would never run on this platform; one keyed on this predicate does — which + // is the entire fix `model_loader.cpp`'s CheckDeviceWeightFit call site makes. + CHECK_FALSE(rocm_like.needs_weight_staging()); + CHECK(rocm_like.allocates_bounded_device_memory() != + rocm_like.needs_weight_staging()); + + // The default delegation is a SEPARATE code path from this override, so + // proving the override wins is not enough on its own — confirm the DEFAULT + // really would have answered false here (matching needs_weight_staging()), + // by reading it through a platform that overrides neither, already pinned + // above via `cpu` and `sm121`. Restated here for locality: the override on + // this class is what makes the two diverge, not a change to the default. + CHECK_FALSE(GetPlatform(DeviceType::kCPU).allocates_bounded_device_memory()); +} + +// BACKEND-ROCM, issue #1934. The ROCm leg's real answers, exercised only where +// a real ROCm platform is registered (a HIP build on a machine with a device — +// this project's own gfx1200 box qualifies). `needs_weight_staging()` is +// UNCHANGED by this row (still false, on purpose — see rocm.cpp); the new +// method and the real probe are what move. +TEST_CASE("ROCm leg values (real hardware, HIP build only)") { + if (!HasPlatform(DeviceType::kROCM)) return; // no device: nothing to assert + Platform& rocm = GetPlatform(DeviceType::kROCM); + + CHECK_FALSE(rocm.needs_weight_staging()); + CHECK(rocm.allocates_bounded_device_memory()); + CHECK(rocm.allocates_bounded_device_memory() != rocm.needs_weight_staging()); + + // The probe actually ran: a real card reports a nonzero total, and 0 would + // read as UNKNOWN at the load-time refusal — silently disabling it again, + // the exact shape #1934 reports. `hipMemGetInfo` failing on a REGISTERED + // platform (device already proven present by `HasPlatform` above) would be + // its own bug, not an acceptable "0 == unknown" outcome here. + const ResidencyPolicy policy = rocm.residency_policy(); + CHECK(policy.device_memory_total_bytes > 0); + // Neither of the two fields this row deliberately left at their safe + // default (see rocm.cpp's residency_policy() comment) moved. + CHECK_FALSE(policy.release_host_weights_after_upload); + CHECK_FALSE(policy.uses_device_memory_pool); +} + // The CUDA leg's capability ANSWERS, exercised only where a real CUDA platform is // registered (the dgx CUDA build / a GPU box). This is the executable proof that // each S3-converted gate is byte-identical: the predicate returns `true` on this From 6ce8f5dcf2f9d3c0a41dc6d62affc7e10ec01d79 Mon Sep 17 00:00:00 2001 From: Justin Card Date: Wed, 26 Aug 2026 18:03:21 -0400 Subject: [PATCH 3/3] record(BACKEND-ROCM): the outcome -- real hardware, both directions, same box Records the before/after vllm-cli transcript on this gfx1200 card (no refusal -> named refusal, same command, same file, VT_DEVICE_WEIGHT_BUDGET_BYTES=1) and the rejected alternatives (flipping needs_weight_staging(), reusing Backend::DeviceMemoryInfo) with the reasons. Also notes #2021/#2022, the unrelated GCC 15 build break this row's own verification needed applied and reverted locally, never landing on this branch. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-sonnet-5 [claude-code] --- .../specs/rocm-device-fit-bounded-memory.md | 50 ++++++++++++++++++- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/.agents/specs/rocm-device-fit-bounded-memory.md b/.agents/specs/rocm-device-fit-bounded-memory.md index c315d02fc..a0b182561 100644 --- a/.agents/specs/rocm-device-fit-bounded-memory.md +++ b/.agents/specs/rocm-device-fit-bounded-memory.md @@ -234,5 +234,51 @@ registrar probes the device itself elsewhere in this same file. ## Now -`ACTIVE`. Spec committed; implementation, tests and the real-hardware repro -follow in this same pull request. +`DONE`. Implementation, tests, CPU and real-hardware (gfx1200) evidence are +in. This row closes #1934 and unblocks #1870, which stays open only pending +the operator's merge of this PR (per that issue's own record). + +## Outcome + +**What was measured.** `RocmPlatform::needs_weight_staging()` gates the ONE +production call site of `CheckDeviceWeightFit`. It hardcodes `false`, so the +refusal never ran on ROCm — confirmed directly, before this row, with +`VT_DEVICE_WEIGHT_BUDGET_BYTES=1` (a budget nothing could satisfy) against a +real checkpoint producing no refusal and proceeding to the next load stage. +Fixed by adding `Platform::allocates_bounded_device_memory()`, a narrower +predicate `RocmPlatform` overrides to `true` while leaving +`needs_weight_staging()` itself untouched, and a real, once-probed +`ResidencyPolicy::device_memory_total_bytes` via a new HIP-free +`vt::rocm::DeviceMemoryTotalBytes` free function. + +**Real hardware, both directions, same command, same file, same box.** +`vllm-cli --model /models/Bonsai-27B-Q1_0.gguf --device auto --max-tokens 4` +with `VT_DEVICE_WEIGHT_BUDGET_BYTES=1`: + +- BEFORE (stashed the fix, rebuilt): no refusal, 2.72s to reach an unrelated + dequant error three load-stages later. +- AFTER: refuses in 0.28s — + `device 'rocm' cannot serve this GGUF: staging its weights needs at least + 3787168768 bytes (3.52 GiB) of device memory across 851 tensors, ... and + this device's memory pool is 1 bytes (0.00 GiB)`. + +**What was rejected.** Flipping `needs_weight_staging()` itself — considered +first, and rejected once reading `IndexedGdnStateIoEnabled` showed it already +takes ROCm's fast arm today via an op-registration check that flipping the +flag would DELETE (see Design), and that at least one sibling consumer +(`MergedGdnBaEnabled` and its relatives) has no such fallback and would +activate unconditionally on unverified kernels. Reusing +`Backend::DeviceMemoryInfo` directly from the platform registrar — rejected +for the static-init-order hazard the registrar's own existing comment already +names, and because it is a live per-request probe with a different, currently +CUDA-dead consumer (issue #1126) that reusing it would silently wake. + +**An unrelated pre-existing build break was found and fixed separately, not +in this row.** A fresh CPU build of the claim base (`2a42cb369`) failed under +GCC 15's `-Werror=nonnull` in Tenstorrent debug-dump code +(`qwen3_5.cpp:4019`), unrelated to ROCm or this row's files. Filed as #2021 +and fixed as its own tiny PR (#2022, `BACKEND-TENSTORRENT-QWEN35`) rather than +riding this branch, per the same one-issue-one-unit-of-work reasoning this +spec applies to itself — this row's own verification needed it, so it was +applied locally and reverted before every commit in this branch, never +landing here.