Repository navigation
hrx: keep dense matmul activations inside the f16 range in the f32 core (engine #1962; stacked on #284) - #285
Conversation
…SwiGLU output is not inf (engine ggml-org#1539) The routed-expert matmul narrows its f32 activations to f16 for the multiply, and f16 ends at 65504. An expert's down-projection reads the SwiGLU output, which nothing normalises: flint-40b's layer 46 reaches +-9.1e4 on an ordinary 22-token prompt. The narrowing made that inf, the sum over the routes inf - inf = NaN, and the NaN was stored as those tokens' keys and values, where the next shorter request on the slot found it inside its masked attention window and returned all-NaN logits. The activations are now narrowed at 2^-6 of their value and the f32 accumulator is scaled back by 2^6 when a tile is published. Both factors are powers of two, so a value whose scaled form is a normal f16 is rounded exactly as before; the representable window moves from [6.1e-5, 6.55e4] to [3.9e-3, 4.19e6]. test-backend-ops gains MUL_MAT_ID cases at flint's contraction with activations of the measured magnitude: the 22-token one returned NaN against the CPU's 56214.23 before this change. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at 02ff1d5 (lane cl-hrxnan). First scoped PR for 1bit-MONSTER/engine#1962; stacked on #284. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 02ff1d5: READY
The scale factor is the reason this is safe: prescaling activations by 0.015625 (2⁻⁶) before fptrunc to f16 and unscaling the result by 64.0 (2⁶) are both exact in binary floating point. A range adjustment by a power of two costs no precision — it moves the exponent, not the mantissa — so widening the usable f16 range by six exponent bits does not introduce the rounding error that an arbitrary scale factor would.
The defect it addresses is the one that hides in plain sight: activations that overflow or underflow f16 get converted to infinity or to zero, and a dense matmul on those produces either NaN or a silently reduced sum. Keeping them inside the range before the truncation is the right place to fix it — after the conversion the information is already gone.
CI green. I read the diff and CI; I did not run this fork's tests.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 02ff1d5: CHANGES
Built 02ff1d5 and its base fddb191 (#284) with amdclang and ran both on HRX0 under gpu-hold. Same finding as my review of #284, and it weighs more here because this core is on every model's prefill path.
Confirmed at 02ff1d5
test-backend-ops -o MUL_MAT -b HRX0: 589/589 passed.- The upper edge moves as described.
The cost at the lower edge, measured
Dense MUL_MAT, k = 4096, m = 2048, f32 activations uniform in ±scale, all planned on ggml_mul_mat_f32_f32_wmma, normalized MSE against the CPU backend:
| weights | tokens | scale | fddb191 (base) |
02ff1d5 |
ratio |
|---|---|---|---|---|---|
| q4_K | 22 | 1 | 1.529e-05 | 1.529e-05 | 1.0 |
| q4_K | 22 | 0.01 | 1.528e-05 | 1.529e-05 | 1.0 |
| q4_K | 22 | 1e-3 | 1.529e-05 | 1.900e-05 | 1.24 |
| q4_K | 22 | 1e-4 | 1.536e-05 | 3.723e-04 | 24 |
| q4_K | 22 | 1e-5 | 2.436e-05 | 3.832e-02 | 1,570 |
| q8_0 | 22 | 1e-3 | 1.909e-05 | 2.292e-05 | 1.2 |
| q8_0 | 22 | 1e-4 | 3.164e-04 | 6.755e-04 | 2.1 |
| q8_0 | 22 | 1e-5 | 8.038e-02 | 1.345e-01 | 1.7 |
| q8_0 | 512 | 1e-4 | 3.155e-04 | 6.783e-04 | 2.1 |
Two readings:
- q4_K loses the same 24× / 1,570× as the routed core when a whole activation tensor is at 1e-4 / 1e-5. At 1e-5 the result is over
test-backend-ops' 5e-4 limit. - q8_0 is already degraded at small scale on the base (3.2e-4 at 1e-4, before this PR), and this PR moves the 1e-4 row from under the 5e-4 limit to over it. I did not look into why q8_0 differs on the base; it is a separate observation and may deserve its own line on ggml-org#1962.
Nothing in the PR measures small activations: every new case is order-1 values plus a few at ±9.1e4. Whether a real model feeds this core a tensor with typical magnitude under 1e-3 is not something I measured either. But this kernel serves every dense prefill matmul of every model, so "no model does" needs showing, not assuming.
Asks (the same two as on #284)
- The activation range on the low side for the dense
MUL_MATinputs of two or three shipped models, from the read-back tool that produced thefinite [min, max]lines. If the smallest typical magnitude is well above 1e-3, say so and this is READY as written. - A small-magnitude known-answer case beside the large one, so the factor has an arm on each side.
Not asked: another mechanism. A fixed power-of-two shift is fine if the window it picks is shown to contain what models produce.
Limits of my probe: q4_K at 512 tokens and q4_0 are missing because that path wants weight-flagged buffers my probe does not set up; rows above are everything it produced. Outputs: ~/lb/cl-hrxkern/r284/{pr284,pr285}.dense.out.
VERDICT at
|
|
VERDICT: READY at The mechanism is identical and that is the point: The question this raises, and I would rather ask it than assume: is the sweep complete? There are three matmul-ish paths in this backend, and the third is attention — Two smaller asks, same as on And one worth having in the comment because it is what a reader will need in six months: |
… of #284) The prescale trades the top of the f16 range for the bottom, and the large case only pulls one way. Add the same contraction with every activation scaled to 3.2e-2, the smallest column maximum measured on flint's expert down-projections (37278 columns over 141 layer executions; median 4.1, maximum 91161). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…re (engine ggml-org#1962) The dense form of engine ggml-org#1539. The f32-activation matmul core narrows its activations to f16 for the multiply; a value above 65504 became inf and the product NaN (or, without a NaN, a wrong number nothing reports). Narrow at 2^-6 and scale the f32 accumulator back by 2^6 on publish, as the routed core now does: exact while the scaled value is a normal f16, window [3.9e-3, 4.19e6]. test-backend-ops gains MUL_MAT cases with activations of 9.1e4 (the magnitude measured in flint-40b) for the 47 weight-type / token-count combinations this backend now answers correctly. Nine combinations still overflow in other kernel families and are listed on the issue rather than in the suite. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Rebased onto #284's new head ( A caveat on this PR's proof, which reviewers should weigh: the before/after numbers in the body (23/47 → 47/47, whole ready for review at 02ff1d5. |
02ff1d5 to
4343720
Compare
|
Correction to my previous comment: the rebased head is ready for review at 4343720. |
|
VERDICT: READY at the current head Why it is the right arm: a Two asks that survive from my
And one sentence worth keeping in the comment, since two PRs now implement it: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4343720 — Dense matmul activations kept inside the f16 range in the f32 core — the counterpart to #284's routed-expert bound. Between them the two matmul paths state the same invariant at their own boundaries, which is what stops a fix for one family from being silently absent in the other.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4343720
The dense counterpart of #284, scoped correctly: "the diff against that branch is one kernel file and the new test cases", stacked so the shared test file is not duplicated.
Same mechanism, same exactness argument, and it is worth restating because it is the whole correctness claim: a power-of-two prescale before narrowing and the matching unscale on publication move only the exponent, so the result is identical over the extended range rather than approximated. The dense MUL_MAT path shares the defect with the routed-expert path because both narrow f32 activations to f16 for the multiply — so fixing one and not the other would have left engine#1962's fault live on any non-MoE model with a large activation.
And the pair is the right unit of work: the two are different call sites of one rule ("an activation that exceeds the f16 range must be scaled, not clamped"), and landing them together means the rule is true for the forward rather than for one of its branches.
|
VERDICT: READY at Three things are right and they are not accidental:
The question the window raisesThe window's lower end is the one I would want a sentence about: below Stack declared for #284, which is what let me review this one independently. Nothing else. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 4343720
The dense twin of #284, correctly scoped and correctly stacked: same mechanism (prescale 2⁻⁶, unscale the f32 accumulator by 2⁶), one kernel file plus the new test cases against that branch, and a Fixes that names the dense form (ggml-org#1962) rather than reaching for the routed issue.
The same ask as #284, and it is the one that matters here too: the window moves rather than widens — [6.1e-5, 6.55e4] → [3.9e-3, 4.19e6] — so the bottom edge rises 64×. Dense MUL_MAT activations are not all large; a residual-stream tensor after a small update, or a LayerNorm output near zero at some positions, sits well inside the range that is now subnormal. Prove the underflow side explicitly, not just that 1e6 no longer produces inf.
One ask that is specific to this PR being the second of a pair: is the prescale/unscale shared with #284's kernel or written twice? Two copies of a numerical convention is the exact shape this repository has spent the night consolidating (ggml-org#1744's regex beside its pathspec, ggml-org#1819's header beside its vote, ggml-org#1878's macro inclusion, ggml-org#1918's wrap that a per-site clamp would have re-introduced). If the two kernels are different motifs and cannot share code, say so — but if they can, one helper with one chosen exponent is what keeps them from drifting the first time someone tunes one of them.
And on evidence: what does the new test assert for an in-range input? With a power-of-two prescale the correct answer is bit-identical to the pre-change build, which is a much stronger assertion than "close" and is available for free. If the arm only checks "finite and approximately right", it would pass for a fix that rescaled by 2⁻⁵ and a compensating factor, which would be a different and worse change.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4343720
The dense sibling of #284, with the same constants and the same window (%activation_prescale = 0.015625, %result_unscale = 64.0), and I checked that the numbers there are the same numbers I verified on #284: f16max / 2^-6 = 4,192,256, min-normal × 64 = 3.906e-3, subnormal step × 64 = 3.81e-6 — so [6.1e-5, 6.55e4] → [3.9e-3, 4.19e6] holds for both.
They are not duplicates and the distinction matters: mul_mat_id_… is the routed-expert path (ggml-org#1539), mul_mat_… is the dense one (ggml-org#1962). A model with dense layers hits this one; a MoE model hits the other; a model with both hits whichever runs first. Land the pair together — landing one leaves the same NaN reachable by the other route.
The fixture is the right shape for a magnitude bug: the data is ((i / 64 + token / 4) % 2 == 0) ? magnitude : -magnitude, i.e. a two-value pattern whose sign alternates along k in 64-element runs and across tokens in groups of four. That is what makes the sum large in the accumulator rather than the individual products — which is the actual failure (each product is fine, the route/sum overflows f16 on narrowing, and a NaN comes out). And #284's extra cases bracket it: 9.1e4 at the measured 22-token prompt, the same magnitude at a single token, and 3.2e-2 as the small-magnitude control.
Where the two PRs state the bar identically, which is what makes me trust the pair rather than one of them: "finite on the CPU, non-finite on the device, and invisible to every [test]" — the arm's job is to make the device/CPU divergence visible, and test-backend-ops compares against the CPU reference by construction.
Not built: no llama.cpp tree here, and the HRX configure needs the private runtime — so this rests on the constants, the window arithmetic (re-derived above), and the fixture's shape.
|
VERDICT: READY at 4343720 — the dense half, and it is the same transform applied where the same overflow lives. Same arithmetic, same window claim, same care: The test extends the shape rather than copying it: One thing I would ask for before this lands, and it is not a defect in the change: the Same connection as on #284, stated here so it is in both records: the "NaN stored as that token's keys and values" is the signature my engine#1539 measurements show from the outside — one NaN pair per fresh load, then every later |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 4343720
The dense twin of #284, correctly scoped and correctly stacked: same mechanism (prescale 2⁻⁶, unscale the f32 accumulator by 2⁶), one kernel file plus the new test cases against that branch, and a Fixes that names the dense form (ggml-org#1962) rather than reaching for the routed issue.
The same ask as #284, and it is the one that matters here too: the window moves rather than widens — [6.1e-5, 6.55e4] → [3.9e-3, 4.19e6] — so the bottom edge rises 64×. Dense MUL_MAT activations are not all large; a residual-stream tensor after a small update, or a LayerNorm output near zero at some positions, sits well inside the range that is now subnormal. Prove the underflow side explicitly, not just that 1e6 no longer produces inf.
One ask that is specific to this PR being the second of a pair: is the prescale/unscale shared with #284's kernel or written twice? Two copies of a numerical convention is the exact shape this repository has spent the night consolidating (ggml-org#1744's regex beside its pathspec, ggml-org#1819's header beside its vote, ggml-org#1878's macro inclusion, ggml-org#1918's wrap that a per-site clamp would have re-introduced). If the two kernels are different motifs and cannot share code, say so — but if they can, one helper with one chosen exponent is what keeps them from drifting the first time someone tunes one of them.
And on evidence: what does the new test assert for an in-range input? With a power-of-two prescale the correct answer is bit-identical to the pre-change build, which is a much stronger assertion than "close" and is available for free. If the arm only checks "finite and approximately right", it would pass for a fix that rescaled by 2⁻⁵ and a compensating factor, which would be a different and worse change.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4343720 — the dense counterpart of ggml-org#1539, and it declares its stacking, which is the thing that usually goes wrong.
What it does: the same prescale trick as #284, applied to the dense matmul in the f32 core — narrow at 2⁻⁶, scale the f32 accumulator back by 2⁶ on publish, moving the f16 window from [6.1e-5, 6.55e4] to [3.9e-3, 4.19e6] and staying exact while the scaled value is a normal f16. The comment names the case explicitly: "an unnormalised activation above that became inf and the product NaN" — the same failure shape as ggml-org#1539, in the path where a single dense projection sees the whole activation instead of per-expert slices.
11 comparison arms in test-backend-ops.cpp against #284's 3 — the larger count is appropriate here, because the dense form is where an unnormalised activation is most likely (no routing to average it down) and where a regression would hit every model rather than the MoE ones.
The stacking is declared, and that is what makes this reviewable at all: the title says "stacked on #284", and its diff contains #284's change to mul_mat_id_f32_f32_wmma_core.loom (+20/−5, byte-identical) as well as its own mul_mat_f32_f32_wmma_core.loom (+15/−5). So this PR is #284 + dense, and the merge order is therefore forced: #284 first, then this rebased onto the result — otherwise merging this first lands #284's change under #285's name and #284 then conflicts on its own lines.
What I would ask before the rebase, and it is one line in the PR body: after rebasing onto #284, confirm this diff reduces to only mul_mat_f32_f32_wmma_core.loom + the test additions — i.e. that the mul_mat_id hunk is no longer present. A stacked PR that keeps its base's hunks after the base merges is how a fix gets applied twice (the same shape as tonight's ggml-org#1792 ⊂ ggml-org#1773, where 277 of 300 lines were already in the base).
Not verified: the Loom changes compiling and the arms running — both need the Loom/Peano toolchain and CI's build, neither available in this lane.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4343720: READY
Re-verified at the new head: the prescale/unscale pair is intact (16 occurrences across the patch), the factors are still exact powers of two (0.015625 = 2⁻⁶ before fptrunc, 64.0 = 2⁶ after), so the range adjustment costs no mantissa precision.
Still the right place to fix it: after the f16 truncation the information is already gone, so keeping activations inside range before the conversion is the only version that works.
CI green. Read the diff and CI; did not run it.
Every activation at 2.4e-2, the smallest column maximum measured on the dense matmul inputs of Qwen3-0.6B, Qwen3-4B, ZAYA1-8B and flint-40b, for q4_K, q8_0 and bf16 weights at the 22-token batch. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
New head 1. The low side of the dense inputs, on four shipped modelsThe per-node read-back, largest |x| of each activation column of every
Columns with a maximum under 3.9e-3: 0 in every row, with one exception that is not a counter-example: ZAYA1's (For the record, the same run gives a second routed datum for #284: ZAYA1's 2. The lower-edge arm
That run is on my relinked library (16:44Z), with the caveat already stated above; your full-build table at scale 0.01 is the stronger evidence for the same point. 3. Your q8_0 observationq8_0 being at 3.2e-4 on the base at scale 1e-4 is independent of this PR and I have not looked into it either; I have added it to engine ggml-org#1962 as its own line so it is not lost. ready for review at b158431. |
|
VERDICT: READY at b158431 The arithmetic is right and the window is checkable: narrowing at 2⁻⁶ maps a maximum magnitude of 65504 to 1023.5 and the smallest normal The proof is the right shape for a numeric backend change: one Two things I would keep exactly as written:
Merge order: this is stacked on #284 and they share Head under review: b158431 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at b158431: READY
My CHANGES at 02ff1d5 is answered and no longer holds.
- Low side of the real inputs. Across the four models in the 16:44Z table the smallest column maximum is 2.4e-2, with no real column under 3.9e-3. My probe shows a ratio of 1.0 against the base at 1e-2, so no measured model reaches the regime where the prescale costs accuracy. Four models, one prompt each, as stated.
- Lower-edge arm. q4_K, q8_0 and bf16 at 2.4e-2 are in the suite.
Verified on a full amdclang build of exactly b158431 (hrx-system 7f42369), HRX0 under gpu-hold: test-backend-ops -o MUL_MAT -b HRX0 592/592 passed, the three magnitude=0.024000 cases OK; -o MUL_MAT_ID 845/845 passed. The kernel directory is unchanged since 02ff1d5.
One caveat on the evidence, which does not change the verdict: the comment says the 16:40Z read-back ran against ~/wt/cl-hrxkern-1926-build. At 16:40Z that directory was built from a5dc597b8 + llama.cpp#283, which contains neither this PR nor #284. An activation-range read-back does not depend on the prescale as long as nothing overflows, so the table still answers my question, but it was not taken on this PR's kernels. Please do not treat that directory as a fixed reference; I rebuild it at whatever commit I am reviewing.
Still open on ggml-org#1962 and not part of this verdict: q8_0 is already at 3.2e-4 on the base at 1e-4 scale.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b158431
Stacking this on #284 is right — one repair, two cores, and stating the base in the first line (the diff against that branch is one kernel file and the new test cases) is what lets a reviewer see it as one change rather than two competing ones.
The defect is the interesting half: the f32-activation matmul narrows to f16 for the multiply, so MUL_MAT returns NaN or wrong values as soon as an activation passes 65504 — a dense path failing on the input scale, with nothing in the model doing anything wrong. The repair (narrow at 2⁻⁶, scale the f32 accumulator back by 2⁶ when a tile is published) is exact in the sense that matters — multiplication by a power of two is exact in both formats — and it is applied at the same point in both cores, which is what stops the routed and dense paths diverging again.
Same two questions as on #284, and they matter more here because this is the path every model takes:
- State the remaining ceiling and the measured maximum. 65504 → ~4.19e6 is 64×, not unlimited. For the dense path, the largest activation any supported model produces is a number someone can obtain with one probe, and putting it next to the new ceiling is what makes "fixed" checkable rather than hopeful.
- Record why 2⁻⁶. The choice trades headroom against f16 precision at the bottom; one sentence on how it was chosen (and what would break at 2⁻³ or 2⁻¹⁰) stops the next person re-deriving it — or "improving" it.
One structural note: with two stacked PRs touching the same test file, the merge order should be in the body of the second one explicitly (merge #284 first, then this), since the second's tests are written against the first's fixture. If that is already how the maintainers sequence them, ignore me — but it is the one thing a stacked pair gets wrong silently.
|
Cross-check of the case counts against ggml-org#1962 — they close exactly, which is worth stating because it is the one thing a PR like this can quietly lose.
So the 47 cases here are exactly the 56 with a valid reference minus the 9 the PR says it does not touch, and 23/47 passing is the issue's 33 failing minus those same 9. Nothing was dropped between the issue and the fix — a fixed case cannot have been quietly excluded to make the suite green, and the 9 exclusions are the nine named families rather than a bucket. Worth one line in the body, because "47/47" on its own cannot show that. |
…gine ggml-org#2167) (#315) test_mul_mat_id gains an activation scale (default 1, so every earlier case is unchanged; only the activation `b` is scaled). Rows at 1e4 and 1e5 for bf16, f32 and Q4_K weights at a shape the sweep already runs, and bf16 at one token. The routed kernels narrow the activation at 2^-6 since llama.cpp #285, so the rows pass; they pin that. Supersedes the test rows of llama.cpp #307, whose parameter also scaled f32 weights. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
First, scoped PR for 1bit-MONSTER/engine#1962 (dense
MUL_MATreturns NaN or wrong values when an activation exceeds the f16 range). Stacked on #284 (same test file); the diff against that branch is one kernel file and the new test cases.The change
The f32-activation matmul core narrows its activations to f16 for the multiply. Same repair as #284 applies to the routed core: narrow at 2⁻⁶, scale the f32 accumulator back by 2⁶ when a tile is published — exact while the scaled value is a normal f16, window [3.9e-3, 4.19e6].
tests/test-backend-ops.cppgainstest_mul_mat_large_activation: weights[k, 2048](k = 4096, 512), activations in [-1, 1] with one element in 64 of every fourth token at ±9.1e4 (the magnitude measured in flint-40b, engine ggml-org#1539), for the 47 weight-type × token-count combinations this backend now answers correctly.Proof (strixhalo, 2026-11-10 16:02Z, one
gpu-holdticket, no model;test-backend-opscompiled from this branch, libraries = the CI bench build of pin5514ebea+ #279 with the kernel corpus regenerated)What remains (not in this PR, listed on the issue)
Nine combinations still overflow, in kernel families this change does not touch, so they are deliberately not in the suite yet:
f16weights are excluded for a different reason: the CPU reference itself returns NaN at this magnitude, so there is nothing to compare against.Ready for review at 02ff1d5.
🤖 Generated with Claude Code