vulkan: fix turbo4_0 block layout and centroid tables (sync with C reference) - #204
Merged
TheTom merged 3 commits intoJul 5, 2026
Conversation
…nd cases turbo3 had a SET_ROWS quantize/dequant round-trip test and an entry in the FLASH_ATTN_EXT type_KV sweep; turbo4 had neither. The FA cases upload CPU-quantized blocks to every backend, so they catch block-layout drift between ggml-common.h and a backend's shader-side struct (the class of bug fixed for Metal in b01afef). On Vulkan these cases fail at feature/turboquant-kv-cache tip 558c6b7 and pass with the layout fix. Signed-off-by: Christopher Maher <chris@mahercode.io>
…KV layout mismatch) 77ab7e9 shrank block_turbo4_0 to 66 bytes (norm + qs[64]) in ggml-common.h, and b01afef applied the matching fix to Metal, but the Vulkan shaders kept the legacy 68-byte struct (norm + rnorm + qs[64]) and copy_to_quant.comp still wrote the removed rnorm field. Every Vulkan shader indexing block_turbo4_0 therefore strode the KV cache at 68 bytes/block while the C side allocates, sizes, and byte-copies it at 66, corrupting turbo4 KV data whenever a C-layout accessor (uploads, state save/restore, defrag copies) and shader-layout addressing met, and running out of bounds near the top of the buffer at large n_kv. Observed on gfx1151 (Strix Halo, RADV) serving GLM-4.7-REAP-218B with --cache-type-k/v turbo4 --ctx-size 131072: generation loses EOS deep into a multi-turn session (runaway 12-16k-token completions), then llama-server SIGSEGVs. Same build with f16 KV is stable. The new turbo4 FLASH_ATTN_EXT and SET_ROWS_TURBO4 backend tests fail before this change and pass after. Signed-off-by: Christopher Maher <chris@mahercode.io>
77ab7e9 re-derived the turbo4 Lloyd-Max centroids (KLD/PPL fix) and updated the C, CUDA, and Metal tables, but the Vulkan shaders kept the old values (+-0.173926 ... vs +-0.241529 ...). After the block-layout fix, every turbo4 FLASH_ATTN_EXT case on Vulkan still failed against the CPU reference with a uniform ~40% relative error, matching the 1.39x table ratio. Updates the FA dequant macro centroids and the copy_to_quant quantizer centroids/midpoints to the C reference values. Signed-off-by: Christopher Maher <chris@mahercode.io>
TheTom
merged commit Jul 5, 2026
337f08e
into
TheTom:feature/turboquant-kv-cache
11 of 25 checks passed
Owner
|
Merged, thanks @Defilan. Great catch that Vulkan never got synced when 77ab7e9 reworked turbo4, and the diligence here (test-backend-ops going 0/528 -> 528/528, plus the 64-minute soak reproducing and then clearing the gfx1151 SIGSEGV) made this an easy review. Appreciate the thorough writeup in #203 too. |
TheTom
pushed a commit
that referenced
this pull request
Aug 4, 2026
TQ3_1S had no Vulkan shaders at all -- unlike TQ4_1S, whose shaders existed but were never compiled (348dac47b). Add dequant_tq3_1s.comp and mul_mat_vec_tq3_1s.comp, then register them the same way TQ4_1S was. Derived clean-room from this repo's own CUDA (ggml-cuda/mmvq-tq.cu, turbo-quant.cuh) and Metal (ggml-metal.metal kernel_mul_mv_tq3_1s_f32_impl) implementations plus the C reference in ggml-turbo-quant.c. Format: 16-byte block, d0 (elements 0-15) + d1 (16-31) + 12 bytes holding 32 three-bit indices as 4 groups of 8-in-3-bytes. The 8 Lloyd-Max centroids are ASYMMETRIC, unlike TQ4's mirrored 16, which is the exact drift #204 had to fix for turbo4; the tables here were checked byte for byte against TQ3_0_CENTROIDS / TQ3_0_SIGNS / TQ_INV_SQRT32 in ggml-turbo-quant.c, and against their CUDA and Metal copies. Indices are unpacked by reading each 3-byte group as a 24-bit little-endian word and shifting, matching tq3_extract_index() in mmvq-tq.cu. This is exact rather than incidental: the C packer lays index i at bits [3i, 3i+2] of that word, including indices 2 and 5 which straddle a byte boundary. Verified exhaustively against the reference bit-field unpack over all 2^24 possible groups. The mat-vec kernel never dequantizes a weight. With H the 32x32 symmetric Hadamard realised by the butterfly, S = diag(signs) and k = 1/sqrt(32), a stored block c dequantizes to w = k*S*H*c, so <w, x> = k*<H*c, S*x> = k*<c, H^T*S*x> = k*<c, H*S*x> and the rotation moves onto the activation: sign-flip and butterfly the activation block once per workgroup in shared memory, then dot against raw centroid*scale straight out of memory. That turns one WHT per (block, row) into one per (block, column). The identity is written down nowhere in the repo, so it is spelled out in the shader -- the absence of an inverse WHT in a mat-vec kernel is otherwise indistinguishable from a bug. Registration mirrors 348dac47b exactly: explicit string_to_spv() rather than adding "tq3_1s" to type_names, because that loop would emit USE_SUBGROUP_ADD variants (wrong on gfx1151's wave64 -- the shader maps one thread per element of a 32-element block and is only correct at a 32-thread workgroup), an unused mul_mat_vec_id, and a get_rows via get_rows_quant.comp that applies no inverse WHT. Host side pins tq_wg_size = 32 with a SHMEM reduction. GET_ROWS support is deliberately not claimed, and TQ3_1S is excluded from the coopmat2 paths (there is no dequant_funcs_cm2.glsl entry; gfx1151 is coopmat1 only, and a stub returning zeros would be worse than no support). dequant_tq3_1s.comp deviates from dequant_tq4_1s.comp in one respect: its butterfly is a flat 16-iteration loop per stage with a compile-time constant stride, not the rolled triple loop. The rolled form has a non-constant inner increment, so glslang cannot unroll it and buf[] stays a dynamically indexed function-local array -- scratch, on a shader that runs over the entire weight tensor during prefill. The flat form compiles to zero loops and no function-local array, and produces bit-for-bit identical results (checked against dequantize_row_tq3_1s over 4096 random blocks). dequant_tq4_1s.comp still has the rolled form; fixing that belongs in a separate change. tests: 258 TQ3_1S MUL_MAT cases mirroring the TQ4_1S sweeps -- 240 mat-vec cases at Gemma-4 dimensions plus 18 large-batch cases that force the dequant + f16 matmul path, which is the only coverage the inverse WHT gets. With the 10 cases the existing all_types sweeps already produced, that is 268 TQ3_1S MUL_MAT cases total, matching TQ4_1S exactly. 120 of them use type_b=f16. Not yet validated on hardware. Local verification was: all three shaders compile under glslc -O and pass spirv-val; the emitted block stride is 16 bytes, matching sizeof(block_tq3_1s); the mat-vec SPIR-V contains no subgroup capability at all; and a C transliteration of both shaders reproduces the CPU reference bitwise. The f16 activation pipeline is registered but cannot be checked by test-backend-ops, whose CPU comparison backend does not run type_b=f16. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes two independent bugs in the Vulkan
turbo4KV cache path that 77ab7e9's turbo4 rework left unsynced on the Vulkan backend:rnormfield from the Vulkanblock_turbo4_0shader struct (types.glsl) and the write to it (copy_to_quant.comp), bringing the Vulkan layout back to the 66-byte C struct (norm+qs[64]). Mirrors the Metal fix in b01afef.flash_attn_dequant.glsl,copy_to_quant.comp) with the re-derived C reference (CENTROIDS_4BIT/nearest_centroid_4bitinggml-turbo-quant.c), which 77ab7e9 also updated for C/CUDA/Metal but not Vulkan.Also adds turbo4 to the
SET_ROWSround-trip andFLASH_ATTN_EXTcross-backend sweep intest-backend-ops.cpp(turbo3 already had both; turbo4 had neither), which fail on Vulkan before either fix and pass after both.Why
Every Vulkan shader indexing
block_turbo4_0was striding the KV cache 2 bytes long per block while the C side allocated and byte-copied at 66 bytes, and dequantizing against a centroid table about 40% off the current one. On gfx1151 (Strix Halo, RADV) serving GLM-4.7-REAP-218B with turbo4 K/V at ctx 131072 under sustained multi-turn load, this corrupted attention as context grew (completions lose EOS and run away 12-16k tokens), then SIGSEGV'dllama-server. The same build with f16 KV is stable.Fixes #203.
Validation
test-backend-opson gfx1151 (RADV STRIX_HALO): turbo4FLASH_ATTN_EXTwent from 0/528 passing (444inf mismatch, 84ERR ~= 1.0) before either fix, to 0/528 with a uniform ~40% relative error after the layout fix alone, to 528/528 passing after both fixes (full sweep 9509/9509,Backend Vulkan0: OK). turbo3 control unaffected throughout (528/528OK). Full breakdown in turbo4 KV on Vulkan: block layout and centroid tables out of sync with C reference (KV corruption, runaway generation, SIGSEGV on gfx1151) #203.