Findings of a four-angle review of src/backends/vulkan/ (altitude), left over after the cleanup PR. All verified by reading the code; nothing here is a reproduced failure — these are robustness/maintainability debts. File/line references are to main after #460.
1. A failed flush is not sticky (small, worth doing first)
vk_seq_flush is void; a failed submit/wait prints to stderr, sets an error string and drops the batch, but the op still returns GEIST_OK (sequence.c). vk_argmax_f32 then reads whatever the staging buffer held and returns it as the token; vk_seq_roll ignores EndCommandBuffer/QueueSubmit failures; the resolver-installed linear kernels are void, so the only signal is a zeroed output (AGENT.md §5). → a sticky st->failed, [[nodiscard]] flush, checked at every sync point (argmax, buffer_map, download) → GEIST_E_BACKEND. A device-lost/OOM event should not yield a plausible wrong token.
2. "Try GPU, else CPU" turns real errors into silent fallbacks
vk_try_ew2/ew3 return bool (== GEIST_OK), and the same shape is in vk_rmsnorm, vk_rope_apply, vk_hadamard_rotate, vk_attention (×2), vk_rmsnorm_add, vk_scale_f32: "not applicable" and "failed" are one boolean, so a descriptor-allocation or command-buffer failure is redone on the CPU with a stale error string left on be. → three-way result (DONE / NOT_APPLICABLE / ERROR) or split each op into a geometry predicate and a dispatch, so only the predicate can route to a fallback.
3. vk_fused_supported and the ops disagree
The contract says probe true ⇒ the op must succeed, and a bound ffn_norm_gate_up returning UNSUPPORTED is a hard layer error, but the checks are written twice and have drifted: the GELU_TANH_MUL probe gates on mask bit 1 while vk_try_ew3 uses bit 2; GELU_TANH_MUL_SCALED probes true although vk_gelu_tanh_mul_scaled has no GPU path (a host loop that flushes every layer); the probe keys weights by q->gate_w->raw, the op by buffer->host_alias + offset; xo/nwo alignment, vk_tensor_gpu(t_x) and vk_t_n(y) >= n_out are checked in the op only. → one vk_<op>_check(be, geometry) → reason per op used by both the probe and the entry; a host-only op probes false.
4. No loud choke point for GPU coverage gaps
stat_cpu_falls counts tensors (not ops) accessed through vk_tensor_host, is printed only under GEIST_VK_VERBOSE and is asserted nowhere. It does not see linear_t returning UNSUPPORTED (the arch then takes the synchronous host linear: flush, memcpy, dispatch, flush), vk_w_cpu_mN, the qgate/rope_il/deltanet/argmax/kv_append UNSUPPORTED returns or vk_buffer_copy's host branch; resolve_weight moves F16/BF16/Q3_K/misaligned-row weights to the CPU row-dequant path without a load-time note. → one vk_fallback(st, op_id, reason) helper at every fallback site with per-op counters, optional GEIST_VK_STRICT=1 that turns a fallback into an error, one load-time summary line ("N weights, X MB on the CPU dequant path"), and the resident-model tests (test_bonsai_e2e_int, test_qwen35_vulkan_e2e_int) asserting zero fallbacks. A coverage regression currently shows up only as a slowdown (cf. #409/#410).
5. GEIST_VK_GPU_OPS / VK_OPS(be, bit) is an undocumented relic
28 uses in ops.c with unnamed magic bits (1, 2, 4, … 256) and a raw & 32u in resources.c; no test/script/CI/doc sets it; the bits are already inconsistent with the probes (item 3). Delete it, or replace it by the fallback counters/strict mode of item 4 (they report what actually ran instead of switching ops off). GEIST_VK_NO_BARRIER (sequence.c, "WRONG results", an atomic + getenv in a per-dispatch function) is the same kind of leftover.
6. Static limits are checked per call instead of once
VK_XRING_CAP (192 MB) is enforced in vk_xring_stage on every call and caps.max_m = 512 is never checked against cap × the widest n_in — a too-large batch turns into false → UNSUPPORTED → a silent slow host linear; xring and argmax_out are created lazily on the hot path; the "subgroup_size != 32" rule is written in three places (see #471); 102 unchecked (uint32_t) narrowings in ops.c, and vk_tensor_gpu casts byte_off / 4 unchecked (AGENT.md §3). → size and allocate the ring from max_m and the widest n_in at create/plan time (fail the load if it does not fit), select the mm pipeline once in resolve_weight, a checked vk_u32().
7. C and GLSL constants must agree by hand
Embed dtype codes (ops.c vs DTYPE_* in embed_lookup_scaled.comp), VK_ROPE_IL_MAX_HEAD_DIM = 256 vs sx[256], VK_HADAMARD_MAX_BLOCK = 1024 vs sh[1024], the literals 512/192/128 in vk_attention/vk_attn_qkv_prep vs CHUNK in attn_part_f16.comp, rows/batch rows per workgroup in vk_linear_gx/gy vs the shaders' NUM_ROWS/tile sizes, and positional uint32_t[] push blocks (hadamard's push[11] with a float memcpy'd into slot 10). A wrong constant is a silent GPU miscompute. → one limits header included by C and GLSL (#include works with glslc), a vk_dtype_code() shared by resolve and embed, a named push struct per shader with static_assert(sizeof == …).
8. Weight registry and buffer lifetime rely on caller discipline
The registry is keyed by host pointer only (no size/dtype), entries live until backend destroy (VRAM grows across model reloads), vk_weight_lookup has 16 call sites with three key derivations (w->raw, host_alias + offset, the embed table pointer), aliased buffers copy the parent's VkBuffer without a refcount, and a failed hostbufs grow silently skips registration (later aliased slices lose their GPU binding and fall to the CPU path). → resolve_weight stores the device buffer in the geist_weight (removes the lookup and the key derivations; also a perf win, see #469), a release_weight hook in the vtable, registry OOM as an error, parent-outlives-alias enforced.
9. vk_w_cpu_mN breaks the hot-path contract
Allocates (heap_alloc_aligned) per call against "linear_m1/mN are allocation-free" (include/geist_weight.h, AGENT.md §3), returns with y unwritten on OOM (§5), single-threaded double-precision naive dot. A large BF16/F16 output.weight would run this way with no indication (see item 4). → scratch in vk_state or at resolve, an error status, a load-time note; long term F16/BF16 shaders.
Acceptance
Per item: a test that fails before (e.g. a forced submit failure → GEIST_E_BACKEND; a probe/entry agreement test extending test_fused_probe_agreement_unit to Vulkan; GEIST_VK_STRICT=1 over the Bonsai/qwen35 e2e).
Findings of a four-angle review of
src/backends/vulkan/(altitude), left over after the cleanup PR. All verified by reading the code; nothing here is a reproduced failure — these are robustness/maintainability debts. File/line references are tomainafter #460.1. A failed flush is not sticky (small, worth doing first)
vk_seq_flushisvoid; a failed submit/wait prints to stderr, sets an error string and drops the batch, but the op still returnsGEIST_OK(sequence.c).vk_argmax_f32then reads whatever the staging buffer held and returns it as the token;vk_seq_rollignoresEndCommandBuffer/QueueSubmitfailures; the resolver-installed linear kernels arevoid, so the only signal is a zeroed output (AGENT.md §5). → a stickyst->failed,[[nodiscard]]flush, checked at every sync point (argmax,buffer_map, download) →GEIST_E_BACKEND. A device-lost/OOM event should not yield a plausible wrong token.2. "Try GPU, else CPU" turns real errors into silent fallbacks
vk_try_ew2/ew3returnbool(== GEIST_OK), and the same shape is invk_rmsnorm,vk_rope_apply,vk_hadamard_rotate,vk_attention(×2),vk_rmsnorm_add,vk_scale_f32: "not applicable" and "failed" are one boolean, so a descriptor-allocation or command-buffer failure is redone on the CPU with a stale error string left onbe. → three-way result (DONE/NOT_APPLICABLE/ERROR) or split each op into a geometry predicate and a dispatch, so only the predicate can route to a fallback.3.
vk_fused_supportedand the ops disagreeThe contract says probe true ⇒ the op must succeed, and a bound
ffn_norm_gate_upreturning UNSUPPORTED is a hard layer error, but the checks are written twice and have drifted: theGELU_TANH_MULprobe gates on mask bit 1 whilevk_try_ew3uses bit 2;GELU_TANH_MUL_SCALEDprobes true althoughvk_gelu_tanh_mul_scaledhas no GPU path (a host loop that flushes every layer); the probe keys weights byq->gate_w->raw, the op bybuffer->host_alias + offset;xo/nwoalignment,vk_tensor_gpu(t_x)andvk_t_n(y) >= n_outare checked in the op only. → onevk_<op>_check(be, geometry) → reasonper op used by both the probe and the entry; a host-only op probes false.4. No loud choke point for GPU coverage gaps
stat_cpu_fallscounts tensors (not ops) accessed throughvk_tensor_host, is printed only underGEIST_VK_VERBOSEand is asserted nowhere. It does not seelinear_treturning UNSUPPORTED (the arch then takes the synchronous host linear: flush, memcpy, dispatch, flush),vk_w_cpu_mN, the qgate/rope_il/deltanet/argmax/kv_append UNSUPPORTED returns orvk_buffer_copy's host branch;resolve_weightmoves F16/BF16/Q3_K/misaligned-row weights to the CPU row-dequant path without a load-time note. → onevk_fallback(st, op_id, reason)helper at every fallback site with per-op counters, optionalGEIST_VK_STRICT=1that turns a fallback into an error, one load-time summary line ("N weights, X MB on the CPU dequant path"), and the resident-model tests (test_bonsai_e2e_int,test_qwen35_vulkan_e2e_int) asserting zero fallbacks. A coverage regression currently shows up only as a slowdown (cf. #409/#410).5.
GEIST_VK_GPU_OPS/VK_OPS(be, bit)is an undocumented relic28 uses in
ops.cwith unnamed magic bits (1, 2, 4, … 256) and a raw& 32uinresources.c; no test/script/CI/doc sets it; the bits are already inconsistent with the probes (item 3). Delete it, or replace it by the fallback counters/strict mode of item 4 (they report what actually ran instead of switching ops off).GEIST_VK_NO_BARRIER(sequence.c, "WRONG results", an atomic +getenvin a per-dispatch function) is the same kind of leftover.6. Static limits are checked per call instead of once
VK_XRING_CAP(192 MB) is enforced invk_xring_stageon every call andcaps.max_m = 512is never checked against cap × the widestn_in— a too-large batch turns into false → UNSUPPORTED → a silent slow host linear;xringandargmax_outare created lazily on the hot path; the "subgroup_size != 32" rule is written in three places (see #471); 102 unchecked(uint32_t)narrowings inops.c, andvk_tensor_gpucastsbyte_off / 4unchecked (AGENT.md §3). → size and allocate the ring frommax_mand the widestn_inat create/plan time (fail the load if it does not fit), select themmpipeline once inresolve_weight, a checkedvk_u32().7. C and GLSL constants must agree by hand
Embed dtype codes (
ops.cvsDTYPE_*inembed_lookup_scaled.comp),VK_ROPE_IL_MAX_HEAD_DIM = 256vssx[256],VK_HADAMARD_MAX_BLOCK = 1024vssh[1024], the literals 512/192/128 invk_attention/vk_attn_qkv_prepvsCHUNKinattn_part_f16.comp, rows/batch rows per workgroup invk_linear_gx/gyvs the shaders'NUM_ROWS/tile sizes, and positionaluint32_t[]push blocks (hadamard'spush[11]with a floatmemcpy'd into slot 10). A wrong constant is a silent GPU miscompute. → one limits header included by C and GLSL (#includeworks with glslc), avk_dtype_code()shared by resolve and embed, a named push struct per shader withstatic_assert(sizeof == …).8. Weight registry and buffer lifetime rely on caller discipline
The registry is keyed by host pointer only (no size/dtype), entries live until backend destroy (VRAM grows across model reloads),
vk_weight_lookuphas 16 call sites with three key derivations (w->raw,host_alias + offset, the embed table pointer), aliased buffers copy the parent'sVkBufferwithout a refcount, and a failedhostbufsgrow silently skips registration (later aliased slices lose their GPU binding and fall to the CPU path). →resolve_weightstores the device buffer in thegeist_weight(removes the lookup and the key derivations; also a perf win, see #469), arelease_weighthook in the vtable, registry OOM as an error, parent-outlives-alias enforced.9.
vk_w_cpu_mNbreaks the hot-path contractAllocates (
heap_alloc_aligned) per call against "linear_m1/mN are allocation-free" (include/geist_weight.h, AGENT.md §3), returns withyunwritten on OOM (§5), single-threaded double-precision naive dot. A large BF16/F16output.weightwould run this way with no indication (see item 4). → scratch invk_stateor at resolve, an error status, a load-time note; long term F16/BF16 shaders.Acceptance
Per item: a test that fails before (e.g. a forced submit failure →
GEIST_E_BACKEND; a probe/entry agreement test extendingtest_fused_probe_agreement_unitto Vulkan;GEIST_VK_STRICT=1over the Bonsai/qwen35 e2e).