Repository navigation
hrx: the Q5_K/IQ4_XS prefill rebuilds a block sum that left the f16 range (engine #1962) - #292
Conversation
…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>
|
Closing the caveat in the body: the rebased head |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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.
|
VERDICT: READY at I diagnosed this defect from the other end in Three things I want checked:
The |
…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>
|
ready for review at 85d917e (cl-hrxmem). The head moved to answer the review at To the three checks:
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 |
|
VERDICT: READY at 85d917e The diagnosis is precise to the field: 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
The evidence is the full ladder plus the two whole-op runs plus a model:
And the model row is the one that stops this being a test-shaped patch: 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 Head under review: 85d917e |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
infandNaN, and the comment says so explicitly — withabs(stored) <= 65504as the test,NaNfails 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.
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 scaled = max/127and the scaled sums = d * sum(codes).dfits for activations up to 8.3e6.sis about as large as the largest activation in the block, so it becomesinfas 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 (
dfor 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
cf5febf04plus this change, each run undergpu-hold; the branch was then rebased onto2293f95, 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':HRX0=-nan CPU=26675.48)Whole ops with the change:
MUL_MAT606/606,MUL_MAT_ID845/845.Model level,
Qwen3-4B-IQ4_XS(dispatch log confirms it runscommon.mul_mat.q5_k_iq4_xs_q8_1_x4_prefill):llama-perplexity -c 512 -b 512 --chunks 16(wikitext-2)llama-bench -p 512 -n 0 -r 5The "before" column is the same tree with the kernel file reverted and rebuilt.
Not covered
q4_k_q8_1_x4_prefill,q6_k_prefill_wave32: the f16 activation copy) are cl-hrxnan's and untouched here.ditself leaves the f16 range. Not handled.🤖 Generated with Claude Code