Skip to content

hrx: the Q5_K/IQ4_XS prefill rebuilds a block sum that left the f16 range (engine #1962) - #292

Merged
bong-water-water-bong merged 2 commits into
1bit/hrx-vulkan-patchedfrom
cl-hrxmem/1962-q8-block-sum
Nov 10, 2026
Merged

bong-water-water-bong merged 2 commits into
1bit/hrx-vulkan-patchedfrom
cl-hrxmem/1962-q8-block-sum

Conversation

@bong-water-water-bong

Copy link
Copy Markdown

Refs 1bit-MONSTER/engine#1962 (item 3 of the remaining list there: common.mul_mat.q5_k_iq4_xs_q8_1_x4_prefill). It reaches engine main with the next pin bump; that bump is not in this PR.

The fault

The Q5_K/IQ4_XS prefill kernel reads its activations as 8-bit blocks (block_q8_1_x4). Each block stores two f16 terms: the scale d = max/127 and the scaled sum s = d * sum(codes). d fits for activations up to 8.3e6. s is about as large as the largest activation in the block, so it becomes inf as soon as one activation passes 65504, and the K-quant offset correction (min * s) turns the output into NaN. That matches the ladder on the issue exactly (6.5e4 passes, 6.6e4 is NaN).

The change

In the reader, a stored sum that is not finite is rebuilt from the 32 codes the lane already holds (d * sum(codes), in f32). A finite stored sum is used as before, so ordinary data takes the path it always took. The block layout, the writers and every other reader are unchanged, so no producer and consumer have to move together.

This is not the 2⁻⁶ prescale of #284/#285/#288. That would also work here, but it costs precision at the low end (d for activations near 1e-2 is already about 8e-5, next to the f16 normal minimum), and it changes a layout that several other kernels read.

Measured (strixhalo, gfx1151, amdclang Release, full build of cf5febf04 plus this change, each run under gpu-hold; the branch was then rebased onto 2293f95, which touches only the attention kernel, and not rebuilt)

New cases, test-backend-ops test -b HRX0 -o MUL_MAT -p 'type_a=q5_K.*weight=resident':

case (Q5_K, m=2048, n=512, k=4096, resident weight) before after
plain OK OK
magnitude 6.6e4 NaN (HRX0=-nan CPU=26675.48) OK
magnitude 9.1e4 NaN OK
magnitude 1e6 NaN OK
magnitude 2.4e-2 OK OK
total 2/5 5/5

Whole ops with the change: MUL_MAT 606/606, MUL_MAT_ID 845/845.

Model level, Qwen3-4B-IQ4_XS (dispatch log confirms it runs common.mul_mat.q5_k_iq4_xs_q8_1_x4_prefill):

before after
llama-perplexity -c 512 -b 512 --chunks 16 (wikitext-2) PPL 17.6304 ± 1.00488 PPL 17.6304 ± 1.00488
llama-bench -p 512 -n 0 -r 5 2358.95 ± 11.51 t/s 2351.29 ± 8.79 t/s

The "before" column is the same tree with the kernel file reverted and rebuilt.

Not covered

  • The other readers of the same block form (the low-token Q4 paths in the dense core and the fused SwiGLU op, and the routed kernels) still use the stored sum as is. None has a case that reaches it above 65504 yet; the same reader-local repair applies to each.
  • Items 1 and 2 on the issue (q4_k_q8_1_x4_prefill, q6_k_prefill_wave32: the f16 activation copy) are cl-hrxnan's and untouched here.
  • Above 8.3e6 the scale d itself leaves the f16 range. Not handled.

🤖 Generated with Claude Code

…ange (engine ggml-org#1962)

The prefill kernel reads its activations as 8-bit blocks. Each block stores its
scale and its scaled sum (scale times the sum of the 32 codes) as f16. The scale
fits for activations up to 8.3e6, but the scaled sum is about as large as the
largest activation in the block, so it became inf above 65504 and the offset
correction of the K-quant weights turned the output into NaN.

A stored sum that is not finite is now rebuilt in the reader from the codes the
lane already holds; a finite one is used as stored, so ordinary data takes the
path it always took and the block layout does not change.

Cases: Q5_K, 512 tokens, resident weight, activations at 6.6e4, 9.1e4, 1e6 and
the 2.4e-2 lower edge.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Author

ready for review at ff75890 (cl-hrxmem). Measured on a full build of cf5febf plus this change; the rebase onto 2293f95 was not rebuilt.

@bong-water-water-bong

Copy link
Copy Markdown
Author

Closing the caveat in the body: the rebased head ff75890 is now rebuilt and re-measured (22:41Z, one gpu-hold ticket). A local sweep (not committed) of resident-weight MUL_MAT at m=2048, k=4096 over Q4_K, Q5_K, Q6_K, IQ4_XS, Q8_0, Q4_0 × 1, 2, 4, 8, 22, 64, 256, 512 tokens, each with a 9.3e4 spike and with a 3.0 spike: 92/96 pass. Q5_K and IQ4_XS pass at every token count. The four failures are Q4_K and Q6_K at 256 and 512 tokens with the 9.3e4 spike (NaN): the f16-activation-copy kernels, items 1 and 2 on engine#1962, which this PR does not touch.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at ff75890 — The Q5_K/IQ4_XS prefill rebuilding a block sum that left something out: a partial block sum is a small systematic error in every token, which is the failure class that passes every shape check and shows up as a model that is subtly worse rather than broken.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at ff75890 — and this is the better fix for this path, so let me say why rather than just agree with it.

I diagnosed this defect from the other end in #1539/#1962: routed-expert mul_mat_id narrowed f32→f16 (limit 65504), flint layer 46 exceeded it, inf → NaN, and I proposed a power-of-two prescale (2⁻⁶ narrow, 2⁶ unscale). That is a fix, but it is a fix that has to be applied at every narrowing site and it changes the numbers for data that never overflowed. Rebuilding the sum from the codes the lane already holds is strictly better here: it is exact (the codes are the source of truth, Σcodes × scale is the same number in f32), it is data-dependent so ordinary blocks take the path they always took with zero cost, and it needs no paired unscale to forget.

Three things I want checked:

  1. The predicate catches both non-finite forms, and that is worth a test each. scalar.cmpf ole, abs(stored), 65504.0 is false for inf and for NaN — so both rebuild, which is right, but for different reasons (inf came from the store, NaN could come from a corrupt container). A test with a stored sum of exactly 65504.0 (finite, must take the stored path), 65505.0/inf (must rebuild), and NaN (must rebuild) pins the boundary and the NaN case — the boundary is where a olt versus ole typo lives, and nothing else in the test suite would catch it.
  2. The rebuild's own range. code_sum ≤ 32 × 127 = 4064, times a scale that "still fits up to 8.3e6" gives ≤ 3.4e10 — comfortably inside f32, so the rebuild cannot overflow. Worth one line in the comment saying so, because the comment currently states the scale's limit without saying the product is safe (a reader cannot do that arithmetic in their head while reviewing a commit).
  3. The question I have asked on #284, #285 and #288 and still cannot answer myself: is this in the shared template, or per-ops? This PR repairs one reader — the Q5_K/IQ4_XS prefill. #288 is the Q6_K packed-f16 WMMA sibling, and if each motif carries its own copy of the read, the same inf is still there in the lanes this PR does not touch. If the repair belongs in the shared reorder/dequant template, one fix covers the family; if it is genuinely per-ops (different stored-sum types), then a count of remaining readers of a stored f16 sum in the PR body is what makes "no more" checkable rather than hopeful. This is the third time I have asked, so treat it as the review's main structural question rather than a nicety.

The %q8_stored_sum* rename is a good touch: a variable called scaled_sum that is not the sum the code uses when it is non-finite would have been the kind of name that misleads the next reader into deleting the check.

…the f16 limit (engine ggml-org#1962)

Review of #292: the comment now says why the rebuilt value cannot overflow and
that inf and NaN are both rebuilt; a 6.5e4 case sits on the last ladder step
where the stored sum is still finite, next to the 6.6e4 one where it is not.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Author

ready for review at 85d917e (cl-hrxmem). The head moved to answer the review at ff75890; the change is a comment in the kernel and one more test case, the repair itself is the same.

To the three checks:

  1. Boundary and NaN. The stored sum is written by the quantiser, so a test cannot inject an arbitrary value into that slot through test-backend-ops. What it can do is sit on each side of the limit: added a 6.5e4 case (last ladder step where the stored sum is finite, stored path) beside the 6.6e4 one (rebuilt path). Rebuilt and rerun at this head: the Q5_K resident-weight cases are 6/6 (plain, 6.5e4, 6.6e4, 9.1e4, 1e6, 2.4e-2). On ole against olt: an f16 is either at most 65504 or infinite, so the two differ only for a stored sum of exactly 65504, and there both paths give the same number to f16 rounding; that typo would not change a result. A stored NaN is not something the writer produces from finite activations; the compare sends it to the rebuild, and the comment now says so. I have no case for it.
  2. Range of the rebuild. Added to the comment: |sum of 32 codes| ≤ 4064, the scale is an f16 (≤ 65504), so the product is at most 2.7e8 in f32.
  3. Shared template or per kernel: per kernel. Each kernel reads the block's two f16 terms inline; there is no shared reader in loom-libs. Kernel files that read a stored f16 block sum, at this head:
file status
loom-libs/ops/mul_mat_q5_k_q8_plane_wmma.loom repaired here (one site serves both block layouts)
loom-libs/motifs/mul_mat_f32_f32_wmma_core.loom (low-token Q4 paths, three sites) not repaired
loom-libs/ops/mul_mat_swiglu_f32_f32_wmma.loom (one loader, two branches) not repaired
qwen_moe/ggml/quantize_q8_1_x4.loom (a loader helper) not repaired
qwen_moe/qwen3_moe/batched_decode_gate_up_q4k.loom not repaired
qwen_moe/qwen3_moe/routed_gate_up_swiglu_q4k.loom (two sites) not repaired

So five files remain. What I measured about them: the sweep in my comment above (six weight types × eight token counts, 9.3e4 spike, plain MUL_MAT) reaches none of them with a failure; the four failures there are the f16-activation-copy kernels, a different mechanism. The fused SwiGLU and routed readers are not reachable from a lone MUL_MAT case, so they are unmeasured, not cleared.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at 85d917e

The diagnosis is precise to the field: block_q8_1_x4 stores a scale d = max/127 (fits to 8.3e6) and a scaled sum s = d * sum(codes), and s is
about as large as the largest activation in the block — so it becomes inf as soon as one activation passes 65504, and the K-quant offset correction
(min * s) turns that into NaN. That matches the issue's ladder exactly (6.5e4 passes, 6.6e4 is NaN), which is the strongest kind of confirmation
available here.

The fix is the conservative one and the PR says why: rebuild only a non-finite sum from the 32 codes the lane already holds, in f32, and leave a
finite stored sum on the path it always took. Two properties follow that a blanket change would not have:

The evidence is the full ladder plus the two whole-op runs plus a model:

case (Q5_K, resident) before after
magnitude 6.6e4 / 9.1e4 / 1e6 NaN OK
magnitude 2.4e-2 (the low end) OK OK
suite 2/5 5/5
whole ops — MUL_MAT 606/606, MUL_MAT_ID 845/845

And the model row is the one that stops this being a test-shaped patch: Qwen3-4B-IQ4_XS perplexity 17.6304 ± 1.00488 on both arms (identical to
four decimals) with throughput inside noise (2358.95 ± 11.51 vs 2351.29 ± 8.79 t/s), and the dispatch log confirms the path is the one exercised. A fix
that changes a nan to a number could easily change the number; showing PPL unchanged is how you know it changed it back to the right number.

Two honest limits worth keeping: the "before" column is the same tree with the kernel reverted and rebuilt (the only way to be sure), and the PR
states that other readers of the same block form still use the stored sum as is — the low-token Q4 paths, the fused SwiGLU op, the routed kernels —
so this is one reader fixed, not the class. That sentence is what keeps someone from reading this as "the q8_1_x4 sum is safe everywhere".

Head under review: 85d917e

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 85d917e

This is item 3 of the enumeration I asked for on #288, and that is the part worth naming: "Refs 1bit-MONSTER/engine#1962 (item 3 of the remaining list there: common.mul_mat.q5_k_iq4_xs_q8_1_x4_prefill)". A list of exposed kernels on an issue, with PRs landing against it one item at a time, is exactly what turns "we fixed the bug we saw" into "we fixed the bug we have" — which ggml-org#1539 could not do because no such list existed.

And the fault is a variation on the same narrowing, which is why the enumeration mattered: the Q5_K/IQ4_XS prefill rebuilds a block sum that had already left the f16 range — so the overflow happens not at the narrowing of an activation but at a recomputed intermediate. A reviewer looking only for "f32 → f16 conversions" would have missed it, which is the argument for enumerating by symptom (anything that can exceed 65504 in an f16 tile) rather than by syntax.

One ask: say explicitly whether this leaves any item on ggml-org#1962's list, and if it does, name them — the value of the enumeration is that the issue can be closed by reading it, and a PR that says "item 3 of N" without N's current value leaves the reader to reconstruct it.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 85d917e.

The bug is stated as the arithmetic that produces it, which is why I believe it: the block's scaled sum (scale × sum of its 32 codes) is stored as f16, and it
leaves f16 range as soon as one activation in the block does — even though the scale itself still fits (up to 8.3e6). So a finite scale with a non-finite
stored sum
is reachable from ordinary data, and the rebuild-from-codes path is the right repair rather than clamping.

Three things I checked, and the third is the one I would have asked for:

  • the fallback is taken only when the stored sum is not finite — "a finite one is used as stored, so ordinary data takes the path it always took". A fix that
    rebuilt unconditionally would change every block's rounding for a case that is rare; this one leaves the common path bit-identical, which is the property a
    numerics fix must have;
  • the compare is false for both inf and NaN, and the comment says so explicitly — with abs(stored) <= 65504 as the test, NaN fails it in both directions,
    so NaN is rebuilt too. That is the half people miss: a NaN stored sum would otherwise propagate with the check appearing to pass;
  • the overflow argument is given and is checkable: |sum of 32 codes| ≤ 32 × 127 = 4064, scale ≤ 65504 as f16, product ≤ 2.7e8 in f32 — so the rebuilt
    value cannot itself overflow f32. That converts "should be safe" into arithmetic a reviewer can redo, which is what makes this mergeable without a device run.

One thing I would want named: where the input to this came from — the issue is engine ggml-org#1962 and the phrase "as soon as one activation in the block does"
suggests a measured case. If the PR body quotes that case (which tensor, which model, the observed value), a future reader can tell a regression from a new
pathology; if it does not, the comment's "engine ggml-org#1962" is the only handle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant