fix: apply rebase-audit fixes

[giveen]

Giveen's Rebase Audit — Final Report

Audit completed: August 4, 2026. Branch: feature/turboquant-kv-cache.

Final Tally

Folder	Count	Description
complete/	366	All verified — applied, superseded, or obsolete
unsure/	0	All 28 resolved
missing/	0	Nothing left

Key Finding

The rebase is significantly better than the original fork branch. Almost every diff fell into one of three categories:

1. Already in the rebase — the shader/algorithm code was present, just with different architecture (WHT instead of dense rotation, block-128 instead of block-32, richer mixed KV support, etc.)
2. Superseded experiment — the diff was an experimental approach that the rebase chose not to adopt (e.g., FMA decode, inline block, zero-LUT select chain, block-32 architecture)
3. Obsolete — the diff modified code that the rebase completely replaced (e.g., old block-128 dense rotation API, old quantize schemes)

Only pipeline wiring corrections were ever needed — never algorithmic changes.

Changes Applied to Codebase (5 commits on giveen-says-sorry)

Commit	Diff	What
37daf4485	—	Stack overflow fix (64KB→direct PRNG) + quality benchmarks docs
50bec9726	3eca09aec53e	turbo4 SET_ROWS tail-block truncation + TURBO_ROT_DIM refactor
9ceab7b17	a494833d0dbf	Vulkan turbo3 dequant/get_rows/cpy pipeline wiring
ba0968be5	ff8bb7394661	Fix turbo3 pipeline corrections (dequant wg, cpy removal, supports_op)
266703a2b	f03d331446e4	Vulkan TQ4_1S weight pipeline wiring

All rebased onto latest feature/turboquant-kv-cache on Aug 4 (force-pushed to giveen/giveen-says-sorry).

Unsure Folder Resolution (28 → 0)

All 28 "unsure" diffs were Metal experiments — every single one was already in the rebase or superseded:

Category	Count	Examples
Already in rebase (or better version)	8	Sparse V dequant (auto-enable), fp16 centroid LUT, 4-mag LUT auto-detect, graph-side WHT, asymmetric K/V FA (richer), turbo2 support, FWHT vs dense rotation
Superseded experiment	20	FMA decode, inline block, zero-LUT select, half4 FWHT, SIMD shuffle, named-register norm, block-32, split LUT, pair LUT, register LUT, deferred norm, fused block dot, direct-extract turbo4, V12 CUDA shmem, turbo4 2+1 bit packing, initial Metal TQ (PR #21), inlined rotation matrices, comment fix

Known Issue: CPU PPL Regression

CPU turbo3 PPL = 8.04 vs q8_0 baseline 6.89 (17% degradation). Cause unknown. Needs separate investigation of CPU quantization/dequant path.

PR #260 — Current State

• 3 commits on giveen-says-sorry, rebased onto latest feature/turboquant-kv-cache
• Conflicts with PR vulkan : add TQ3_1S and register TQ4_1S weight types #259 resolved
• Ready for re-review

• I have read and agree with the contributing guidelines
• AI usage disclosure: yes

[TheTom]

Thanks for doing this audit — reconciling 366 diffs against a rebased tree is grim work and the disposition table is genuinely useful. Reviewed the code half; the audit tally itself I have taken as read rather than re-verifying 366 entries, so treat my comments as scoped to the four changed files.

The rotation change is good

Removing the 64KB float G[TURBO_D * TURBO_D] stack array is worth doing on its own merits: the old code generated into G and then immediately memcpy'd it into turbo_rotation before running QR in place, so writing the PRNG output straight into turbo_rotation is exactly equivalent. Verified rather than assumed — built test-turbo-quant from both trees and the output is byte-identical.

One thing to fix: the static_assert and its comment block are pasted twice (lines 34-45 in the new file, grep -c returns 2). Harmless to the compiler, but it should be one copy.

The SET_ROWS ceiling-division change needs rework

The premise does not hold for the path that matters, and the implementation is unsafe on the path where it would.

In the KV path the tail case cannot arise. llama-kv-cache.cpp:378-392 pads turbo head dims up to a multiple of 128 for exactly this reason, for both K and V:

// For turbo types, pad K head_dim to next multiple of 128 for full WHT groups
if (k_is_turbo && n_embd_head_k % 128 != 0) { ... padded_head_k = ((n_embd_head_k + 127) / 128) * 128; ... }

So nk0 is always a whole number of 128-element groups there, floor and ceiling agree, and no tail is ever dropped.

Where non-aligned input can still reach the kernel, ceiling division is worse than floor. The group loop body unconditionally reads grp_src[0..127] and writes 4 blocks per group, with no clamp. On a partial tail group that is an out-of-bounds read past the row and an out-of-bounds write past the block count — memory corruption where the old code merely skipped the tail. To be clear about evidence: that is a code-reading claim, not a measured one. I tried to demonstrate it with a non-aligned test_set_rows_turbo3 case and my harness tripped an unrelated assert inside the test's own view setup, so I have no runtime proof and am not asserting one.

The new GGML_ASSERT(n_elements % 128 == 0) does not cover it either — it is in ggml_metal_op_turbo_wht, which is TURBO_WHT, a different op from the set_rows kernels the ceiling change touches.

Suggested direction: mirror what Vulkan already does rather than changing the arithmetic. ggml-vulkan.cpp:17990 rejects the op outright:

// turbo shaders use a 128-element block: head_dim must be divisible by 128
if ((op->type == TURBO2_0 || TURBO3_0 || TURBO4_0) && (op->src[0]->ne[0] % 128 != 0)) return false;

Metal's supports_op claims TURBO2/3/4 for SET_ROWS with no such guard (ggml-metal-device.m, the SET_ROWS type switch). Adding the guard there makes the unsupported case fall back to CPU instead of silently truncating or corrupting, and then the ceiling division can go back to floor since it becomes unreachable either way. If you would rather keep ceiling division as belt-and-braces, it needs an explicit bounds clamp in the loop body, and the comment should say the case is unreachable in the KV path.

The CPU PPL regression is the most important line in this PR

turbo3 at 8.04 against a 6.89 q8_0 baseline is a 17% degradation on the fork's flagship feature, and it is currently a footnote under "Known Issue". That deserves its own issue rather than living in a PR description — happy to open one if you have not. I have not verified the number myself and am not disputing it; I would want to know whether it reproduces on GPU or is CPU-path-specific before anyone goes hunting, since that alone halves the search space.

On the recommendations

Agree on porting order, with one adjustment: 8c2e0d878279 (fused TQ mat-vec, 3.4x) is the obvious first CUDA target, but note #259 just landed the Vulkan TQ weight types and deliberately left set_rows_tq4_1s / cpy_tq4_1s_f32 unwired, so if someone picks up the Vulkan side that gap is the natural companion piece rather than the coopmat registration.

Draft is the right state for this. Split the rotation fix out and it can land immediately — it is independently good and I have verified it. The SET_ROWS half wants the guard approach first.

[TheTom]

Took the verified half rather than leaving it queued behind the rest: 37daf4485 is now on feature/turboquant-kv-cache as 0967f4997, cherry-picked with your authorship intact (git log shows you as author, not me).

It went in alone because it turned out to be cleanly separable — that commit contains only the stack fix and docs/quality-benchmarks.md. The duplicated static_assert and the SET_ROWS ceiling division are both in 50bec9726, so none of the parts I raised concerns about came along. Re-verified after the pick: test-turbo-quant output byte-identical to base, Metal build clean, SET_ROWS 159/159.

For your draft: rebase onto the branch tip and 37daf4485 will drop out as already-applied, leaving 50bec9726 as the only thing left to sort — the ceiling division wants the Vulkan-style supports_op guard instead, and the static_assert block needs deduping. Everything else in that commit (the TURBO_D to TURBO_ROT_DIM rename) is fine and can ride along once those two are addressed.

Also, docs/quality-benchmarks.md is a good addition and I want to underline one thing in it: you wrote "we have ZERO quantitative quality data on the actual llama.cpp build" and set a target of turbo3 within 1% of q8_0, flagging >2% as a quality problem. Your own audit then measures 17%. That is the most consequential number anyone has produced about this fork in weeks and it should not stay in a PR description — I will open an issue for it unless you would rather, and the first thing worth establishing is whether it reproduces on GPU or is confined to the CPU path.

[TheTom]

Saw the new push. Three things, one of them time-sensitive.

The branch is now conflicting, and the Vulkan TQ4_1S commit is duplicate work

266703a2b re-lands f03d331446e4, which #259 merged a few hours ago. Your branch predates that merge (21eab71bc is not an ancestor of your head), so GitHub now reports the PR as CONFLICTING/DIRTY, and the two implementations differ — mainline has 8 GGML_TYPE_TQ4_1S references in ggml-vulkan.cpp, your tree has 6.

Your own audit called this correctly ("Merge PR #259 — covers f03d331 + 8ba9f12"), so I think this is just a stale branch rather than a disagreement. Rebase onto the tip and drop 266703a2b; 37daf4485 will also fall out as already-applied. Worth doing before any more work goes on top, because #259's version is the one that has been through the wave32/wave64 validation and the MUL_MAT_ID fix.

The other two Vulkan commits (turbo3 KV pipeline, and the wiring correction) look like genuinely new ground that #259 did not touch — those are worth keeping, and I will review them properly once the branch is rebased and I can see them against current mainline rather than through a conflict.

The PPL data is more interesting than the summary suggests

Two observations that I think change how it should be read:

Where the baselines are sane, turbo4 is at parity. qwen3.6-27b: f16 4.05, q8_0 4.06, q8_0/turbo4 4.07. kwaipilot-35b: f16 4.05, q8_0 4.03, q8_0/turbo4 4.02. That is a good result for the fork's headline feature and it is buried under the OSCAR2 rows.

Where the baselines are not sane, the rows cannot support conclusions. gemma4-12b f16/f16 = 360 and gemma4-26b-a4b f16/f16 = 62456. A healthy model on wikitext does not do that at any chunk count — f16 is the control, and a broken control means rows 1-2, 5-6, 8-9, 11-12, 14-15, 18-19 are measuring something wrong with those specific converted models, not properties of oscar2. The analysis section reads it as "high but expected for small models on tiny context"; I do not think that holds, and it would be worth finding out whether those -rot-kv.gguf conversions are themselves broken before drawing anything from the catastrophic mixed-mode numbers. The 1e8 and 3.9e9 values are alarming enough to chase, but with the control broken you cannot yet attribute them to the mixed-mode path.

Also note this run is -ngl 0, so it is CPU — which makes it adjacent to, but not the same as, the 17% turbo3 CPU regression in your audit. turbo4 being at CPU parity here while turbo3 is reportedly 17% off is itself a useful narrowing: it points at the turbo3 path specifically rather than the CPU quant path generally.

Repo hygiene

ppl_test.sh at the repo root, plus ppl_results/, are test artifacts. Same category as the OSCAR logs in #224. The script is useful — scripts/ is the right home for it — and the results belong in the PR description or an issue rather than committed. Not a blocker, just do not let them ride into mainline.

Rebase first and I will pick the review back up from there.

[giveen]

@TheTom working on the rebase now, the seperate PPL issue, I want to ensure that the fixed issues in the audit doesnt cause a conflict or possible may even resolve it, so I will address it when complete.

[giveen]

@TheTom PR is ready.

[TheTom]

Rebase worked — the stack fix and the SET_ROWS commit dropped out cleanly, and the surviving TQ4_1S commit is no longer duplicate work: post-rebase it adds set_rows_tq4_1s and cpy_tq4_1s_f32, which is exactly the half of #69 that #259 deliberately left open. That is genuinely complementary and worth having.

It does not build with Vulkan enabled, though. On GB10, -DGGML_VULKAN=ON:

build-vk/ggml/src/ggml-vulkan/dequant_tq4_1s.comp.cpp:328:16:
  error: redefinition of 'const uint64_t dequant_tq4_1s_len'
  error: redefinition of 'const unsigned char dequant_tq4_1s_data [3840]'

Root cause is the "tq4_1s" entry added to type_names (line 76). The generic loop at line 798 emits dequant_tq4_1s, and #259's explicit call at line 903 emits it again, so the same symbol is embedded twice. Same thing happens host-side: pipeline_dequant[GGML_TYPE_TQ4_1S] is now registered at both line 5277 and 5279, byte-identical.

The fix is small — drop "tq4_1s" from type_names and delete the duplicate host registration, keeping your new set_rows/cpy wiring, which does not depend on the type_names membership.

Worth reading #259's commit message on this specifically, because it predicted exactly this class of problem and the reasoning is still current. Beyond the duplicate-symbol break, that loop also emits for tq4_1s:

• mul_mat_vec_tq4_1s_*_subgroup and _subgroup_no_shmem variants. Dead in your tree — I checked, the 32-thread pin survives your rebase (tq_use_subgroups = false at line 5100), so the host never registers them — but they are wave-size-unsafe by construction for these kernels, so it is better that they not exist at all.
• get_rows_tq4_1s via get_rows_quant.comp, which applies no inverse RHT. Also unreachable today since supports_op does not claim GET_ROWS for TQ types, but it means the wrong implementation now exists as a compiled blob. If anyone later adds GET_ROWS to supports_op, it silently picks that up and returns un-rotated values. That is precisely the trap Defilan flagged for whoever wired this next, and it turns out to be us.

So: drop the type_names entry, keep everything else. I will rerun the Vulkan build and the full sweep on wave32 as soon as it compiles.

Two smaller things while you are in there:

• ppl_test.sh at the repo root and ppl_results/ are still in the diff. scripts/ for the script, and the results are better as an issue comment than committed files.
• Understood on holding the PPL investigation until the audit fixes land, and agreed that is the right sequence. When you do open it, the turbo4-versus-turbo3 split in your own data is a useful starting constraint: at -ngl 0 your table shows q8_0/turbo4 at parity with q8_0 on the two models with sane f16 baselines (qwen3.6-27b 4.07 vs 4.06, kwaipilot-35b 4.02 vs 4.03), while turbo3 is the one reported 17% off. That points at the turbo3 path specifically rather than the CPU quant path in general.

[TheTom]

Builds now, and all three items are fixed — type_names entry gone, single pipeline_dequant registration, ppl_test.sh moved to scripts/ with the results files dropped. Thank you for the quick turnarounds.

GB10, wave32, -DGGML_VULKAN=ON, current head e855ceddf:

op	result
MUL_MAT	1287/1287
MUL_MAT_ID	891/891
SET_ROWS	319/319
CPY	250/250
GET_ROWS	115/115

No failures anywhere, and the 296 TQ3/TQ4 MUL_MAT cases from #259 still pass, so nothing regressed.

But the new wiring is unreachable, so the commit does not yet do what it says. Every tq4_1s SET_ROWS case reports not supported [Vulkan0] — all 24 of them, for both f32 and f16 sources:

SET_ROWS(type_src=f32,type_dst=tq4_1s,type_idx=i64,ne=[256,5,1,3],...): not supported [Vulkan0]

The pipelines are created but supports_op never claims the ops, so dispatch never reaches them. Grepping your tree: GGML_OP_SET_ROWS has no TQ4_1S case, and GGML_OP_CPY has none either. So set_rows_{f32,f16}_tq4_1s* and cpy_tq4_1s_f32 are compiled and registered, then sit dead — the same shape as the get_rows situation, just one door over.

Two things needed to finish it:

1. Add GGML_TYPE_TQ4_1S to the SET_ROWS type switch in supports_op, and the CPY equivalent for the quant-to-f32 direction.
2. Guard the head-dim while you are there. The turbo types in that same switch are gated on src[0]->ne[0] % 128 != 0 because the shaders assume whole 128-element groups; TQ4_1S is a 32-element block type, so its constraint is different, but "no constraint at all" is unlikely to be right. Whatever the real one is, state it explicitly rather than inheriting the absence.

Once those land the tests above will actually exercise the new path instead of skipping it, and I will rerun the same battery — at that point the numbers mean something, which they currently do not for these two ops.

Worth saying: this is a good commit stuck one step short of done, not a wrong one. The shader-gen side, the ne / 32 block-size handling in the copy path, and the pipeline registrations are all correct as far as I can tell; it is only the supports_op claim that is missing.

[TheTom]

The supports_op claim is right and the % 32 guard is the correct constraint for a 32-element block type — thank you for both. Builds clean on GB10 with Vulkan.

Claiming the op made the tests actually run, and the shader fails them. This is the good kind of bad news: the path was never being exercised before, so the green suite was measuring nothing. Now, wave32 / GB10 / test-backend-ops -o SET_ROWS:

ERR = 0.516  SET_ROWS(f16 -> tq4_1s, ne=[256,5,1,3],  nr23=[1,1], r=1, v=0): FAIL
ERR = 0.627  sentinel mismatch: sent_1   (ne=[256,11,1,1], nr23=[2,3], r=7, v=0): FAIL
ERR = 0.644  sentinel mismatch: sent_1   (ne=[96,3,1,1],   nr23=[2,3], r=2, v=0): FAIL
             (ne=[256,5,1,3],  nr23=[1,1], r=1, v=1): OK
ERR = 0.279  sentinel mismatch: sent_1   (ne=[256,11,1,1], nr23=[2,3], r=7, v=1): FAIL
ERR = 0.229  sentinel mismatch: sent_1 sent_2  (ne=[256,5,7,3], nr23=[1,1], r=1, v=0): FAIL
ERR = 1.139  (ne=[256,11,1,7], nr23=[2,3], r=7, v=0): FAIL

6 FAIL, 1 OK. Two things stand out:

1. The errors are structural, not precision. 0.23 to 1.14 against a 0.01 tolerance. A quantizer that was merely lossy would land near tolerance, not 100x past it.
2. sentinel mismatch on four of them means out-of-bounds writes, which is the more serious half — the kernel is writing outside the destination tensor, not just computing the wrong values. Same class as the finding in metal: TQ4_1S MUL_MAT out-of-bounds write (sentinel mismatch) at n=256 with offset #245.

Some signal on where to look, from the shapes rather than the shader:

• The single passing case is nr23=[1,1], r=1, v=0 at ne0=256. Everything with nr23=[2,3] (broadcast) or higher ne2/ne3 fails, so the row/batch indexing looks like the prime suspect rather than the quantization math itself.
• ne0=96 fails, and 96 is a legal multiple of 32, so this is not the guard being too loose.
• v=0 versus v=1 changes the error magnitude but not the outcome, so both the contiguous and view paths are affected.

Worth checking the ne / 32 block-count handling in the copy path against how nb1/nb2/nb3 are strided for the broadcast cases — that combination is where a 32-element block type would most easily diverge from the 128-element turbo assumptions the surrounding code was written for.

No rush on this from my side. When you have a candidate I will rerun the same battery plus the full sweep on wave32, and it would be worth getting @Defilan's eyes on it too if he is willing, since he has the freshest context on how these pipelines are wired.

For the record on process, this is the third time in this repo in a week that a green suite turned out to be skipping the path under test — #259's MUL_MAT_ID at n=16, #242's 0/0-reports-OK, and now this. Your commit is what surfaced it here; the tests were lying before you touched them.

[giveen]

TQ4_1S SET_ROWS Broadcast Fix — Root Cause Analysis

Date: August 4, 2026
Branch: giveen-says-sorry (PR #260)
Commit: ab7c73af9

---

TheTom's Review Comment

The errors are structural, not precision. 0.23 to 1.14 against a 0.01 tolerance.
Sentinel mismatch on four of them means out-of-bounds writes.

The single passing case is nr23=[1,1], r=1, v=0 at ne0=256.
Everything with nr23=[2,3] (broadcast) or higher ne2/ne3 fails,
so the row/batch indexing looks like the prime suspect.

Worth checking the ne / 32 block-count handling in the copy path against
how nb1/nb2/nb3 are strided for the broadcast cases.

Test results (wave32, GB10):

ERR = 0.516  SET_ROWS(f16 -> tq4_1s, ne=[256,5,1,3],  nr23=[1,1], r=1, v=0): FAIL
ERR = 0.627  sentinel mismatch (ne=[256,11,1,1], nr23=[2,3], r=7, v=0): FAIL
ERR = 0.644  sentinel mismatch (ne=[96,3,1,1],   nr23=[2,3], r=2, v=0): FAIL
             (ne=[256,5,1,3],  nr23=[1,1], r=1, v=1): OK
ERR = 0.279  sentinel mismatch (ne=[256,11,1,1], nr23=[2,3], r=7, v=1): FAIL
ERR = 0.229  sentinel mismatch (ne=[256,5,7,3], nr23=[1,1], r=1, v=0): FAIL
ERR = 1.139  (ne=[256,11,1,7], nr23=[2,3], r=7, v=0): FAIL

6 FAIL, 1 OK.

---

Root Cause

The turbo2/turbo3/turbo4/TQ4_1S SET_ROWS Vulkan compute shaders in
ggml/src/ggml-vulkan/vulkan-shaders/copy_to_quant.comp used the source
tensor's element count to compute the total number of workgroups to launch:

if (g >= p.ne / 32) return;   // TQ4_1S  (32-element blocks)
if (g >= p.ne / 128) return;  // turbo2/3/4 (128-element blocks)

p.ne comes from the host-side push constant ggml_nelements(src0) — the total
number of source elements. When the destination tensor has broadcast
dimensions (dst->ne[2] > src0->ne[2] or dst->ne[3] > src0->ne[3]), the
source element count is too small. The shader doesn't launch enough workgroups
to cover all destination blocks, leaving blocks unwritten.

When the test harness later reads back those unwritten blocks, it encounters
uninitialized memory → sentinel value mismatches → out-of-bounds errors.

Example

For ne=[256,5,1,3] with no broadcast (nr23=[1,1]):

• p.ne = 256 * 5 * 1 * 3 = 3840 → 3840 / 32 = 120 workgroups → correct

For ne=[256,5,7,3] with broadcast on dim 2:

• source ne[2] = 1, destination ne[2] = 7
• p.ne = 256 * 5 * 1 * 3 = 3840 → 3840 / 32 = 120 workgroups
• But destination needs (256/32) * 5 * 7 * 3 = 840 workgroups
• 720 blocks never written → sentinel mismatches

---

Fix

Changed all 4 block-based SET_ROWS workgroup guards to use destination
tensor dimensions instead of source element count:

// OLD (broken for broadcast):
if (g >= p.ne / 32) return;
if (g >= p.ne / 128) return;

// NEW (accounts for broadcast expansion):
if (g >= gpr * p.ne21 * p.ne22 * p.ne23) return;

Where:

• gpr = p.ne00 / BLOCK_SIZE (blocks per row)
• p.ne21 = dst->ne[1] (rows)
• p.ne22 = dst->ne[2] (dim 2, includes broadcast expansion)
• p.ne23 = dst->ne[3] (dim 3, includes broadcast expansion)

When there's NO broadcast, p.ne / BLOCK_SIZE == gpr * p.ne21 * p.ne22 * p.ne23
— the formulas are equivalent. The fix only changes behavior when broadcast
dimensions are present.

File changed: ggml/src/ggml-vulkan/vulkan-shaders/copy_to_quant.comp

Affected code paths: SET_ROWS with DATA_A_TURBO2_0, DATA_A_TURBO3_0,
DATA_A_TURBO4_0, and DATA_A_TQ4_1S (4 lines total).

---

Additional Notes

• This also fixes the CPY tq4_1s->f32 path indirectly: the CPY pipeline was
  also claimed in supports_op alongside SET_ROWS in the same commit and
  exercises the same shader entry point through a different preprocessor path.
• The non-block-based generic SET_ROWS path (#elif defined(SET_ROWS)) uses
  get_indices() and the p.ne guard correctly because it iterates element-
  by-element using per-element strides, so broadcast is implicitly handled.

[Defilan]

Ran this on gfx1151 / RADV / wave 64 at the same head (ab7c73af), since a different wave size seemed like the useful thing to add. It reproduces there too, so this is not wave-size specific.

ggml_vulkan: 0 = Radeon 8060S Graphics (RADV STRIX_HALO) (radv) | uma: 1 |
fp16: 1 | bf16: 0 | warp size: 64 | shared memory: 65536 | int dot: 1 |
matrix cores: KHR_coopmat

24 tq4_1s SET_ROWS cases: 4 OK, 20 FAIL.

The 4 passes are vacuous, which makes this worse than it looks

Every passing case is r=1, v=1. v builds a view with r/2 rows (test-backend-ops.cpp:2417), so at r=1 the source has zero rows — ggml_is_empty(src0) is then true and ggml_vk_set_rows early-returns at ggml-vulkan.cpp:13022. Nothing is dispatched, so it matches the reference by writing nothing.

So every case that writes at least one row fails, including the simplest one — one row, no broadcast, contiguous:

SET_ROWS(type_src=f32,type_dst=tq4_1s,type_idx=i64,ne=[256,5,1,3],nr23=[1,1],r=1,v=0): FAIL  ERR 0.246
SET_ROWS(type_src=f16,...same shape...):                                               FAIL  ERR 0.518

I think the bounds check is the cause, and I owe a correction

I first assumed the change from p.ne / 32 to gpr * ne21*ne22*ne23 was a no-op refactor, because p.ne is ggml_nelements(dst) in the unary push-constant initialiser. That was wrong — SET_ROWS uses the binary one, and ggml_vk_set_rows (:13026) passes ggml_nelements(**src0**). I nearly posted the wrong conclusion; flagging it in case anyone else checks the unary path.

With that corrected:

src = [ne0, r,   ne2*nr0, ne3*nr1]     (test-backend-ops.cpp:2409)
dst = [ne0, ne1, ne2*nr0, ne3*nr1]     (:2406)

OLD  g >= p.ne / 32          = gpr * r   * ne2nr0 * ne3nr1     <- source rows
NEW  g >= gpr*ne21*ne22*ne23 = gpr * ne1 * ne2nr0 * ne3nr1     <- dest rows

ne1 is the destination's row count; r is how many rows the source actually has. For ne=[256,5,1,3], r=1, gpr=8: old = 24 groups, new = 120 — five times too many. Every generic case has ne1 > r (5>1, 11>7, 3>2), so they all overrun, reading past src0 and writing destination rows that should be untouched. That fits the errors being structural rather than lossy, and the sentinel mismatches on your side.

Why turbo did not catch this: the turbo types are not in the generic SET_ROWS sweep — they have bespoke test_set_rows_turbo3/4 (:2545, :2612). TQ4_1S is in all_types, so it is the first type through a sweep that varies r independently of ne1. The same edit sits in the turbo paths and is simply unexercised there.

Two smaller things

The summary above says the single passing case was nr23=[1,1], r=1, v=0, but the pasted log line for it reads v=1. Easy slip, and I only noticed because the wave64 run put v=1 in the passing column — but it is what made broadcast look like the discriminator, when the minimal non-broadcast case fails too.

Second, worth a separate look rather than folding into the above: the decomposition does i02 = tmp % p.ne12; i03 = tmp / p.ne12, and p.ne12 is src1's dim 3 (row_idxs is [r, ne2, ne3]), while i02/i03 index src0's dims 2 and 3, which are ne2*nr0 and ne3*nr1. Those only agree in special cases. I have not proven this one causes a failure on its own — the bound explains what I can see — so treat it as a second thing to check rather than a claim.

No sentinel mismatches on this box, across all 20 failures. Since the overrun reads garbage row indices out of data_i, where the resulting write lands is fairly arbitrary, so I would not read much into the two boxes differing there.

Happy to test a candidate fix on wave64 whenever there is one.

[giveen]

@Defilan

Thank you for the detailed trace and the wave64 run — that was exactly the kind of careful review this needed. You were right on all counts.

What was wrong with the first fix

The original p.ne / BLOCK_SIZE bounds check used ggml_nelements(src0) (the source element count). Switching to gpr * p.ne21 * p.ne22 * p.ne23 (destination dimensions) made the dispatch cover broadcast rows, but as you showed, it overruns when ne1 > r — the extra workgroups decompose i03 past p.ne03, reading garbage out of data_s and data_i.

And you corrected my mistaken assumption that the bug was broadcast-specific — the minimal ne=[256,5,1,3], r=1 case fails too, because i03 wraps past source dim 3.

The new fix

After decomposition, before any memory access, all four turbo SET_ROWS paths now check:

if (i01 >= p.ne01 || i02 >= p.ne02 || i03 >= p.ne03) return;

This keeps the destination-based global guard (covering broadcast rows) but prevents source overrun per-workgroup.

The four affected paths in copy_to_quant.comp:

• turbo3_0 (line 426)
• turbo2_0 (line 560)
• turbo4_0 (line 684)
• tq4_1s (line 767)

Secondary decomposition issue

You flagged the i02 = tmp % p.ne12 decomposition where p.ne12 = src1->ne[2] = ne3 doesn't match src0->ne[2] = ne2 when nr0 != 1 or nr1 != 1. The i02 >= p.ne02 arm of the guard should catch any resulting out-of-bounds indices, but I haven't proven the decomposition itself produces correct per-element src/dst pairings in those cases. Worth a separate look but should be memory-safe now regardless.

Request

Happy to get a retest on wave64 whenever you have a moment — same battery: test-backend-ops -o SET_ROWS. The guard should turn the 20 FAIL into 20 bail-outs that leave the destination untouched, matching the reference for untouched broadcast rows.

[Defilan]

Retested 5f5ef77a on gfx1151 / RADV / wave 64. The guard does not change the outcome: still 4 OK / 20 FAIL, same counts as ab7c73af.

The four "passes" are still the r=1, v=1 zero-row cases, so it remains true that every case writing at least one row fails.

What did change is the error magnitudes, on every case:

case	ab7c73a	5f5ef77
f32 ne=[256,5,1,3] nr23=[1,1] r=1 v=0	0.246	0.261
f16 same	0.518	0.272
f16 ne=[96,3,7,1] nr23=[2,3] r=2 v=0	1.220	1.258

So the guard is doing something — it suppresses the overrunning workgroups, which changes what lands in the destination — but the rows that do get written are still wrong.

Why indexing is no longer my suspect

I traced the minimal failing case by hand, ne=[256,5,1,3], nr23=[1,1], r=1, v=0:

src0 = [256, r=1,   1, 3]   ne00=256 ne01=1 ne02=1 ne03=3
src1 = [r=1, ne2=1, ne3=3]  ne10=1   ne11=1 ne12=3
dst  = [256, ne1=5, 1, 3]   ne21=5   ne22=1 ne23=3
gpr  = 8

global bound = 8*5*1*3 = 120, so tmp = g/8 runs 0..14
i01 = tmp % ne01 = 0
i02 = tmp % ne12 = tmp % 3
i03 = tmp / ne12 = tmp / 3

your guard bails i02 >= ne02 (=1), so i02 in {1,2} drops
surviving tmp: 0, 3, 6  ->  i01=0, i02=0, i03 in {0,1,2}

That surviving set is exactly the three real source rows, and src0_idx / src1_idx / dst_idx all resolve in range for them. So for this case the indices are now correct — and it still fails.

That points the remaining defect at the quantize-and-write math rather than the decomposition. It also explains why this never looked broadcast-specific: the simplest possible shape has correct indices and still produces wrong values.

Worth noting the i02 = tmp % p.ne12 mismatch I raised earlier is real but now masked by your guard rather than fixed — in the trace above it only ever selects i02=0 because everything else bails. It is likely to matter again once the value bug is fixed and the broadcast cases actually write, so I would keep it on the list rather than close it.

Suggested next probe

The minimal case is one row of 32-element blocks with correct indices, so the cheapest discriminator is a single-block end-to-end comparison: dump the d0/d1 scales and the 16 packed bytes the shader writes for a known input row, against what quantize_row_tq4_1s_ref produces for the same row. That separates the RHT and scale-search from the packing, and needs none of the broadcast cases.

Happy to run that on wave64 if useful — though you may get there faster on your own hardware, and I would rather not duplicate your work. Also happy to re-run the full battery on any next candidate.

[giveen]

I really need a amd device, i hate having to try to fix things and have no way to test.

[Defilan]

Totally get that. Happy to kick off anything you want me to try. I can dig into it too but don't want to step on your work

[giveen]

I feel like I can only get so far, so if you have suggests or patches, please don't feel like you are stepping on any toes.

[Defilan]

844678bf on gfx1151 / RADV / wave 64. It works. 24 OK, 0 FAIL.

ggml_vulkan: 0 = Radeon 8060S Graphics (RADV STRIX_HALO) (radv) | uma: 1 |
fp16: 1 | bf16: 0 | warp size: 64 | shared memory: 65536 | int dot: 1 |
matrix cores: KHR_coopmat

	ab7c73a	5f5ef77	844678b
tq4_1s SET_ROWS	4 OK / 20 FAIL	4 OK / 20 FAIL	24 OK / 0 FAIL
SET_ROWS suite	323/343, 1/2 backends	323/343, 1/2	343/343, 2/2

And since this touched all four turbo paths, the full sweep to check for collateral: 24560/24560, 2/2 backends, zero failures. CPY is clean too at 252/252. Nothing regressed.

The four cases that used to "pass" were the r=1, v=1 zero-row ones passing vacuously; all 24 now genuinely execute and match the reference.

I owe you a correction, because I steered you wrong

In my last comment I said the remaining defect was in the quantize-and-write math, not the decomposition, and filed the i02 = tmp % p.ne12 mismatch as "a second thing to check rather than a claim". That was backwards. The ne12 → ne02 fix is what resolved everything, and there was no math bug.

I reached that conclusion from a hand-trace of ne=[256,5,1,3], r=1, where I convinced myself your i02 >= ne02 guard already reduced the surviving workgroups to the correct set, so correct indices plus a failing result had to mean the values were wrong. The hardware disagrees, and I have not found the error in that trace. Worth saying plainly rather than quietly moving on, since you might reasonably have spent hours in the scale search on my say-so.

What I should have taken more seriously is that the guard masking a wrong decomposition and the guard fixing it look identical from the outside until you correct the decomposition itself.

On the AMD hardware

Understood, and no need to work blind on this. Send me any candidate and I will run the same battery — build plus test-backend-ops -o SET_ROWS, -o CPY, and the full sweep is about ten minutes end to end. Happy to run a bisect or a one-off instrumented build too if that is more useful than a pass/fail.

Nice fix. The decomposition was the kind of thing that only shows up when a type finally reaches a sweep that varies r independently of ne1.

[TheTom]

Verified on wave32 and merging.

GB10 / NVIDIA / warp size 32, head 844678bf:

check	result
tq4_1s SET_ROWS	24 OK, 0 FAIL
SET_ROWS suite	343/343
CPY	252/252
MUL_MAT	1287/1287
full sweep	24560/24560, zero failures

Identical to @Defilan's wave64 numbers, down to the total. Two wave widths, two vendors, same result — and the four cases that used to "pass" were the vacuous zero-row ones, so this is 24 genuinely executing where 4 were previously measuring nothing.

@giveen — this is the good version of a hard situation. You wrote a shader for hardware you do not have, took the review feedback through several rounds without getting defensive about it, and the thing that finally landed is correct. Saying "I really need an AMD device, I hate having to fix things and have no way to test" is also just true, and worth saying out loud rather than quietly shipping and hoping. The audit that started this PR is what surfaced the whole set_rows/cpy gap in the first place.

@Defilan — thank you for running his patches on your hardware. Catching that the four passing cases were passing vacuously is the kind of thing that only comes from actually reading the output rather than the summary line, and it is the reason this PR ended up correct instead of merely green.

The two of you working the same problem from opposite ends — one with the context, one with the card — got further in an evening than either would have alone, and it is the pattern I would like more of around here.

Merging. The remaining items from the earlier review (the rebase-audit findings, the PPL investigation) are yours to pick up whenever, no timeline.