vulkan : add TQ3_1S and register TQ4_1S weight types - #259
Conversation
|
Correction to the original description, prompted by the question about whether this was a rebase regression: it was, and I had it wrong.
My original wording said the wiring "was never created". That was wrong, and it came from searching history scoped to the branch rather than Two things follow from that:
The TQ3_1S half of the PR is unaffected: that has no prior art in this repo and the validation numbers stand as posted. |
|
Reviewed, and validated on the device class you were missing. This is a well-made PR: the rebase-loss diagnosis is correct, the reasoning for bypassing the generic shader-gen loop is right, and pre-flagging the VerifiedYour rebase-loss claim, independently confirmed. Shader math audited against the in-repo C, CUDA and Metal references — no discrepancies. Block layouts match byte-for-byte (16B tq3_1s, 20B tq4_1s, field order identical); all 8 TQ3 and 16 TQ4 centroids match to the digit across four implementations; the 32-entry sign table matches; the WHT butterfly index formula reproduces the C triple loop pair-for-pair; The 32-thread pin is real and genuinely enforced. wave32 data point — the one you asked for. NVIDIA GB10, RADV-free NVIDIA driver,
So the 32-thread pin holds on wave32 as well as your wave64, and nothing else in the backend regressed from the shader-gen changes. Blocker: MUL_MAT_ID is not actually rejected, and aborts on the decode pathThe PR states MUL_MAT_ID stays rejected by the existing What saved the suite from noticing is a coverage accident: the Add one decode-shaped case next to the existing one and it aborts immediately: // tests/test-backend-ops.cpp, beside the existing n=16 line
test_cases.emplace_back(new test_mul_mat_id(type_a, GGML_TYPE_F32, 4, 2, false, 64, 1, 3*ggml_blck_size(type_a)));Chain: Either fix works and both are small: an explicit Minor, not blockingThe new Fix the MUL_MAT_ID rejection and I will re-run the full battery plus the decode case on wave32 and merge. Nice work on this one. |
|
Thanks for this, particularly the GB10 run. wave32 was the gap I flagged and could not close myself, and having the full sweep clean on a 32-wide device is the evidence this needed. MUL_MAT_IDYou are right, and the mistake was mine rather than an oversight in the code. I checked that the The coverage accident is the part I should have caught. Taken the rejection route, since I have no MoE model staged to validate a
On gfx1151 after the fix: AttributionTaking you up on the offer. A verbatim cherry-pick of #69 is not workable: it predates this branch by 2049 commits, conflicts in 6 of its 9 files, and its two shader files are already in the tree via 2a716ac, so it would try to re-add them. What I have done instead is re-author the first commit as a re-land of #69 with Force-pushed for that reason only. The tree is byte-identical to what you reviewed, so nothing in your audit needs revisiting. Unrelated CI failure
Happy to send a separate one-commit PR casting those to Still outstanding#69's |
The ubuntu arm64 leg builds with -Wdouble-promotion -Werror, where passing float through a variadic is an error: 33 of them in this file. The other legs do not enable it, and the base branch only runs Build Actions Cache, so it stayed invisible until a PR triggered the full matrix (reported by @Defilan in #259). Cast the arguments explicitly; program output is byte-identical.
|
Fix verified on wave32, and your CI report was correct and is now fixed on the branch. MUL_MAT_ID fix — confirmed on GB10 (warp size 32)All four TQ cases now reject cleanly and fall back to CPU, including the decode shape that aborted before: Full sweep: 24496/24496, zero failures — the same number you got on gfx1151, from a 32-wide device. Two architectures, two wave widths, identical counts. I also confirmed the tree is byte-identical to what I reviewed apart from the two expected files, so the force-push needed no re-audit. The comment you left above the rejection is better than the fix itself; anyone who later wires arm64 CI — mine, fixedReproduced on the base branch on an aarch64 box: 33 Two message-only items before I merge
Neither touches code. Fix those and I will merge immediately; no further validation needed from my side. set_rows / cpyLeave it out, and thank you for asking rather than assuming. This PR is a clean, well-scoped unit and I would rather not widen it. Separate PR whenever you feel like it, and if you would rather hand it off, say so and I will take it — the gap is documented well enough in your description that someone else can pick it up. |
Re-lands the Vulkan host wiring from ggml-org#69 (f03d331), which is not an ancestor of the current branch. 2a716ac brought the two TQ4_1S shaders back by a different route without the registration, so dequant_tq4_1s.comp and mul_mat_vec_tq4_1s.comp sit in the tree and are never passed to glslc: the Vulkan SPIR-V build is driven entirely by explicit string_to_spv() calls (there is no glob) and tq4_1s appears in neither type_names nor any generation loop. ggml-vulkan.cpp on the tip has zero GGML_TYPE_TQ4_1S references, so no pipeline is created and every TQ4_1S MUL_MAT falls back to CPU. Same class of loss as 28c68fe, which re-applied the three turbo KV grafts but not the weight types. Not a verbatim cherry-pick of ggml-org#69: it predates this branch by 2049 commits, conflicts in 6 of its 9 files, and its shader files are already present. The approach here is the one Simon reached first, including the fixed 32-thread workgroup, and is credited accordingly. Generated by explicit string_to_spv() calls rather than by adding tq4_1s to type_names, because that loop would also emit USE_SUBGROUP_ADD reduction variants (the shader indexes a 32-entry shared array by gl_LocalInvocationID.x and pairs lanes as (tid, tid + step), so it is correct only at a 32-thread workgroup; RADV on gfx1151 reports warp size 64), a mul_mat_vec_id with no host pipeline, and a get_rows via get_rows_quant.comp which applies no inverse RHT. Does not restore ggml-org#69's set_rows_tq4_1s / cpy_tq4_1s_f32 wiring; that half of the regression is still outstanding. Co-authored-by: Simon Gardling <titaniumtown@proton.me> Assisted-by: Claude
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 ggml-org#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
I claimed in ggml-org#259 that MUL_MAT_ID stayed rejected by the existing mul_mat_id_s/m/l guard. It does not. That guard is a shared-memory heuristic, and ggml_vk_matmul_shmem_support() has no lut_size case for TQ3_1S/TQ4_1S, so it computes a trivially-fitting size and leaves the flags true on mainstream vendors. supports_op therefore returned true for MUL_MAT_ID on both types. That was only survivable because of a coverage gap: the all_types MUL_MAT_ID sweep used n=16, and ggml_vk_use_mul_mat_vec_id() selects the mat-vec path only when src2->ne[1] <= 8. At n=16 it routes to mul_mm_id, which works because pipeline_dequant[TQ*] feeds the generic dequant-to-f16 path, so the suite stayed green while the decode path was never touched. At n<=8 it reaches ggml_vk_get_dequantize_mul_mat_vec_id(), whose type switch has no TQ cases, and asserts on a null pipeline. That is ordinary MoE decode for a TQ-quantized model, not a hypothetical. Reject both types explicitly in the MUL_MAT_ID branch until mul_mat_vec_id_tq{3,4}_1s exists, and add n=1 alongside the existing n=16 in the all_types MUL_MAT_ID sweep so the decode path is covered and the gap cannot reopen silently. Found by TheTom in review of ggml-org#259, including the repro. Assisted-by: Claude
ccb7ecc to
6373003
Compare
21eab71
into
TheTom:feature/turboquant-kv-cache
|
Merged. On reflection, holding this over two commit messages was the wrong call — you had already done the hard part twice over, and asking for another force-push round to move a trailer would have been me optimizing for tidiness at your expense. I carried both in the merge commit instead: Worth saying plainly what this PR did, because it is more than the diff: you found a rebase loss nobody had noticed, explained the wave64 subgroup hazard well enough that the pin is self-documenting for the next person, flagged the Standing offer: if there is another corner of this fork you want to take on, say so and it is yours. |
MoE decode for a TQ3_1S/TQ4_1S model ran entirely on the CPU: #259 rejected MUL_MAT_ID for both types because no mul_mat_vec_id pipeline existed, and reaching ggml_vk_get_dequantize_mul_mat_vec_id() with a TQ src0 asserted on a null pipeline. No new shader source is needed. All of the expert indirection lives in mul_mat_vec_base.glsl (get_offsets(), reduce_result()), which both TQ mat-vec shaders already include, and the expert id arrives via gl_WorkGroupID.y, which neither shader touches. Compiling the existing sources with MUL_MAT_ID adds exactly one binding (the ids buffer, 5 -> 6, matching mul_mat_vec_id_num_bindings) and introduces no subgroup capability, so the 32-thread workgroup pin and the shared-memory butterfly that make these kernels wave64-safe are unaffected. spirv-val passes on both. The blanket reject is replaced by a size gate rather than removed. There is still no TQ mul_mm_id, so prompt processing takes the generic path, where ggml_vk_get_mul_mat_mat_id_pipeline() returns nullptr, qx_needs_dequant goes true, and the ENTIRE expert tensor is staged as f16 -- x_ne is ggml_nelements(src0), across all experts. Reject when that staging buffer would exceed maxStorageBufferRange, or ggml_vk_mul_mat_id_q_f16() reaches GGML_ABORT("Requested preallocation size is too large"). The gate is deliberately independent of src2->ne[1]. An n-dependent gate is worse than no support: weight_buft_supported() (llama-model-loader.cpp) probes with a fixed ids->ne[1] = 512, so a gate that answers differently at load and at decode parks the experts in one backend's buffer and then runs the op in the other, copying every expert tensor across the bus on every token. Worked example of what the gate excludes: DeepSeek-V4-Flash Config-I has ffn_{gate,up,down}_exps of 2048 x 4096 x 256 = 2,147,483,648 elements, so the f16 staging buffer is exactly 4,294,967,296 bytes -- one byte over RADV's UINT32_MAX maxStorageBufferRange. Its experts therefore stay on the CPU, as before this change. Lifting that needs a real TQ mul_mm_id, which reads the quantized data through dequant_funcs.glsl and never allocates the f16 buffer. test-backend-ops gains a TurboQuant MUL_MAT_ID sweep. TQ3_1S/TQ4_1S are in all_types but not base_types, so their only prior coverage was two cases; this covers both sides of the n <= 8 threshold ggml_vk_use_mul_mat_vec_id() splits on, three n_used counts, and broadcast, which exercises the expert-index wrap. Assisted-by: Claude
- vulkan-shaders-gen: add set_rows and cpy_tq4_1s_f32 SPIR-V (kept outside type_names to avoid duplicate dequant symbols and unsafe wave-size mul_mat_vec variants, per PR TheTom#259) - ggml-vulkan: register set_rows, cpy_quant_f32 pipelines; add get_cpy_pipeline and SET_ROWS dispatch; add to get_to_fp16 The rebase already had all shader code: dequant_tq4_1s.comp, mul_mat_vec_tq4_1s.comp, TQ4_1S blocks in copy_to_quant.comp, copy_from_quant.comp, dequant_funcs.glsl, and types.glsl. Only the C++ pipeline wiring was missing. Post-review: dropped \"tq4_1s\" from type_names (PR TheTom#259 already covers dequant/mul_mat_vec via explicit string_to_spv calls), removed duplicate pipeline_dequant registration. Assisted-by: Buffy (Freebuff)
Emits matmul{,_id,_id_subgroup}_tq{3,4}_1s_{f32,f16} from mul_mm.comp plus the
type-independent tq_rotate_act pipeline. Still inert on the host side: no
pipelines are created and no dispatch path selects them yet.
Generated explicitly rather than by adding the types to type_names, matching
the mat-vec precedent in #259 -- that loop would also emit q8_1 mmq variants,
for which these types have no integer-dot path.
coopmat2 is excluded: TQ has no dequant_funcs_cm2.glsl entry, and the target
(gfx1151) exposes KHR_coopmat only.
LOAD_VEC_A is pinned to 8 rather than taken from load_vec_quant, because the
A-side block in mul_mm_funcs.glsl indexes idx/4 and idx&3 to map one invocation
onto exactly one 3-byte packing group of 8 contiguous elements. Any other value
would silently misindex.
One rotate pipeline serves both types: TQ3 and TQ4 share the same 32-element
sign pattern and butterfly, and the shader takes no DATA_A_* define because it
only touches the activation.
Note on shared memory: ggml_vk_matmul_shmem_support() has no lut_size case for
these types, and that is now correct rather than accidental -- the A-side block
uses a function-local const centroid array, not a shared-memory LUT, so its
true LUT cost is zero.
Assisted-by: Claude
2 participants
What
Brings the TurboQuant weight types up on the Vulkan backend. Two commits:
TQ4_1Swiring lost in the rebase. Simon Gardling already did this in vulkan: TQ4_1s support for model weights #69 (f03d331, 2026-04-20). That commit is not an ancestor of 2a716ac, so the current branch carries the shaders without the host wiring:dequant_tq4_1s.compandmul_mat_vec_tq4_1s.compare present but never compiled, because the Vulkan SPIR-V build is driven entirely by explicitstring_to_spv()calls invulkan-shaders-gen.cpp(there is no glob) andtq4_1sappears in neithertype_namesnor any generation loop.ggml-vulkan.cppon the current tip has zero references toGGML_TYPE_TQ4_1S, so no pipeline is created. This is the same class of loss as 28c68fe ("reconstruct supports_op after rebase merge damage"), which re-applied the three turbo KV grafts but not the weight types. This commit adds no shader code.TQ3_1S. Newdequant_tq3_1s.compandmul_mat_vec_tq3_1s.comp, plus thetypes.glsl/dequant_funcs.glslentries and the same host wiring. Ported from the in-tree CUDA (ggml-cuda/mmvq-tq.cu,turbo-quant.cuh) and Metal (ggml-metal.metal) implementations against the C reference inggml-turbo-quant.c.Both are generated by explicit
string_to_spv()calls rather than by adding the type names totype_names, because that loop would also emit three things that must not be used:USE_SUBGROUP_ADD/_NO_SHMEMreduction variants. Both mat-vec shaders map one thread to one element of a 32-element block: they index a 32-entry shared array bygl_LocalInvocationID.x, pair lanes as(tid, tid + step)for the WHT butterfly, and select the half-block scale with(tid < 16 ? d0 : d1). All three are correct only at a 32-thread workgroup, and a subgroup reduction over a wave that is not exactly the workgroup folds in lanes from an unrelated block. RADV on gfx1151 reportswarp size: 64, so this is not hypothetical there. The host side pins these pipelines to a 32-thread workgroup with a shared-memory reduction, independent of the device subgroup size.mul_mat_vec_id_*, for which no host pipeline is created.MUL_MAT_IDstays rejected by the existingmul_mat_id_s/m/lguard insupports_op.get_rows_*viaget_rows_quant.comp, which applies no inverse RHT and whoseget_dm()returnsvec2(1, 0).GET_ROWSsupport is deliberately not claimed for either type; see the note below.Also adds the TQ3_1S MUL_MAT sweeps to
test-backend-ops.cpp, mirroring the TQ4_1S blocks (production dimensions plus the large-batch block that exercises the dequant + f16 matmul path), and the matching tolerance entry.Why
Every TQ4_1S
MUL_MATon Vulkan currently falls back to CPU, and nothing says so. On gfx1151test-backend-ops -o MUL_MATreported991/991 tests passed / OKwhile every TQ4_1S case was skipped rather than run, including the 258 Gemma-4-dimension cases added alongside the shaders, whose comments describe the fused mul_mat_vec kernel andpipeline_dequant[TQ4_1S]as though both were wired. A fully skipped op still reports success (#242), so the gap was invisible from the test suite.The practical cost is in the llama-bench numbers below: prefill on a real TQ4_1S GGUF was running at 1.19 t/s because the matmuls were on the CPU.
TQ3_1S had no Vulkan kernels at all, which is what blocks DeepSeek-V4-Flash Config-I (expert-down, attention, indexer and compressor tensors are TQ3_1S) from running on a 128GB Strix Halo box.
Validation
All on gfx1151,
Radeon 8060S Graphics (RADV STRIX_HALO),warp size: 64,matrix cores: KHR_coopmat.Full
test-backend-opssweep, all ops:Zero FAIL, error or mismatch lines anywhere in the sweep.
MUL_MAT progression, one arm per commit:
991/991(before) to1139/1139(TQ4_1S wiring restored, +148, no new shader code) to1287/1287(TQ3_1S kernels, +148). The TQ4_1S delta is entirely host wiring, which is the cleanest evidence that the shaders themselves were fine and only the registration was missing.llama-bench,
thetom-ai/Nemotron-H-8B-Base-8K-Config-I-GGUF(...-tq4_1s.gguf, 4.82 GiB, 8.10B),-ngl 99 -p 512 -n 128 -r 2, same image base and same GPU on both arms:Worth being precise about what that measures: both arms report
backend: Vulkanand loadlibggml-vulkan.so, so the before arm is on the GPU. Only the TQ4_1S matmuls fall back to CPU per-op via the scheduler. Prefill is matmul-dominated so nearly all of its work was on the CPU, hence 1.19 t/s; decode is bandwidth-bound with more non-matmul work already on the GPU, hence the smaller recovery. This is prefill moving from CPU-bound to GPU-resident, not a kernel getting 388x faster. llama.cpp also printsnemotron_h 31B.A3.5B unknown, may not workfor this arch, unrelated to this change, so the ratio is the meaningful number rather than the absolute t/s.Pre-hardware checks on the TQ3_1S port, since silent numeric corruption is the failure mode for this family: the 3-bit unpack was verified exhaustively against the reference bit-field unpack over all 2^24 possible 3-byte groups; a C transliteration of the dequant shader is bit-for-bit identical to
dequantize_row_tq3_1sover 4096 random blocks; the mat-vec identity holds to 1.2e-8 relative against dequant-then-dot at K = 32/256/1536/2048/3072/4096; and the emitted SPIR-V confirmsArrayStride 16matchingsizeof(block_tq3_1s)(the drift class #204 had to fix for turbo4) with zero subgroup capabilities in the mat-vec module.Not covered
test-backend-ops' CPU comparison backend cannot runtype_b=f16for these types, so 120 of 269 cases per type reportnot supported [CPU]. This is a pre-existing limitation that TQ4_1S shares; I did not want to imply those cases passed.copy_to_quant.comp/copy_from_quant.comphave TQ4_1S branches that are also orphaned today (no cpy shaders are generated for either TQ type). vulkan: TQ4_1s support for model weights #69 wiredset_rows_tq4_1sandcpy_tq4_1s_f32and this PR does not, so that part of the regression is still outstanding. I left it out because I have noSET_ROWS/CPYcoverage for these types on hardware; happy to restore it here if you would rather it land in one go.Note on get_rows
dequant_funcs.glsl's TQ branches returncentroid * scalewith no inverse RHT, andget_dm()returnsvec2(1, 0), which the comment there already says. That is harmless on master precisely because nothing registers aget_rowspipeline for these types, so the path is unreachable. I have kept it that way: nopipeline_get_rows[TQ*], noGET_ROWScase insupports_op. Flagging it because it is a live trap for whoever wiresget_rowsfor a TurboQuant type next, and the genericall_typesGET_ROWS sweep would go green against a wrong implementation.Requirements
ggml-turbo-quant.cbefore submitting. The commits carryAssisted-by:trailers. I am responsible for these changes.