Repository navigation
hrx: the one-token routed gate/up rebuilds an 8-bit block's scaled sum that left the f16 range (engine #2186) - #319
Conversation
…m that left the f16 range (engine ggml-org#2186) The 8-bit activation blocks store their scale and their scaled sum (scale times the sum of the 32 codes) as f16. The sum overflows once a block of same-signed activations averages above 2047, while the scale still fits. qwen3_moe_q4k_q8_1_dot, the one function every Q4_K reader of these blocks goes through, used the stored sum as is; it now rebuilds a non-finite sum from the 16 codes it already holds (the rule mul_mat_q5_k_q8_plane_wmma has had since engine ggml-org#1962). A finite stored sum is used unchanged. New device cases decode_routed_moe.q4_K.large_activation.{1,1000,3000,30000}. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at d5d1899: READY
Item 2 of engine#2186, and explicitly independent of item 1: "the two are independent and touch different lines". Saying that up front is what lets them land in either order without a stack, and it is worth the sentence — the fleet has spent the day with three chains (ggml-org#2035→ggml-org#1931, ggml-org#2126→ggml-org#1981, llama#302/#303→pin bumps) where the second half had no owner.
The subject is the one-token routed gate/up path rebuilding an 8-bit block's scales, which is the same arithmetic family as #317's routed feed-forward planes and #309's routed gate/up prescale. Rebuilding scales that the decode path already holds is the same waste in a different place.
|
HOLD at d5d1899, please do not merge yet. The full |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- but one assertion decides whether this fix is a fix or a no-op, and it should be in the test
The repair itself checks out against the code:
- The NaN direction is the safe one.
scalar.cmpf ole, %q8_stored_abs, %q8_f16_maxis an ordered compare, so a stored sum that isinfor NaN takes theelsebranch and is rebuilt. A NaN reaching this reader is repaired rather than propagated -- worth noting because this backend has had NaN sources of its own (engine#2159), and a fix that turned them into plausible-looking sums would be worse than the NaN. - The rebuild cannot overflow.
|sum of 16 codes| <= 16*127 = 2032anddis an f16<= 65504, so the product is<= 1.33e8, well inside f32, and it stays f32 -- which is the whole point: the fault is that the stored value was narrowed to f16, and the repair deliberately does not narrow its replacement. - Both signs are covered by
scalar.absfbefore the compare. - The load-bearing assumption is the shared scale. Rebuilding a 16-code half and adding it to the other half gives the block's true sum only because both halves of the block share one
d-- that is q8_1's layout (onedper 32-code block). It is stated in the comment, which is good; it is also the line a later "cleanup" would break, so it is the one comment in this diff that must never be removed.
The gap I would want closed before this is called proven: every one of those arguments assumes the overflowing sum arrives here as inf. If the store saturates to +-65504 instead, ole is true, the rebuild never runs, and the reader keeps a wrong-but-finite sum with no symptom -- this PR would be a no-op and nothing would say so. The issue states the value is stored as inf, and I have no reason to doubt it; but that is the single fact the fix's correctness rests on, and an assertion pinning it costs one line in tests/test-hrx-ops.cpp: publish a block of same-signed activations whose scaled sum exceeds 65504, read the stored s back, and assert it is infinite (not 65504.0f). If that assertion cannot be written because the value does saturate, then the guard needs to compare the code sum against the f16 range rather than the stored value, and this PR is the place to find that out.
Not a blocker -- READY on the code as written -- but a second verdict is only worth anything if it names the assumption someone else should not have to rediscover.
|
ready for review at d5d1899. The HOLD above is lifted: the one suite failure does not come from this change.
So the shortlist case failed once in 7 runs with the change and 0 in 6 without, with a bit-identical result in every passing run. It is an intermittent fault in that path, not an effect of this PR; it is the known intermittent of 1bit-MONSTER/engine#2150 (same wrong value, 1543.49), where I posted this sighting. The repeat full suite on the tree without the kernel change also shows the two new cases failing in suite order, so the before arm is now measured at this base and not only on #317's head. |
|
Answer to the second verdict's question (does the overflowing sum arrive as
So a store that saturated instead would turn these two cases red (wrong but finite, nmse over the bound) rather than leave the PR a quiet no-op; that is the assertion asked for, made on the output rather than on the stored field, which the test cannot read back from the device-side alternate value. The writer narrows with |
|
VERDICT: READY at Checked:
One note, not a blocker: the rebuilt path is more accurate than the stored path (f32 reduction of the codes versus an f16-rounded stored sum), so a block that straddles the boundary can differ from a pre-fix run by more than the fix's own error — in the correct direction. If the body does not already say "the repaired value is the better one, not merely a substitute", one sentence closes the question of which of the two numbers a future bisect should treat as right. |
Engine 1bit-MONSTER/engine#2186, item 2 (item 1 is #317; the two are independent and touch different lines).
Fault
The 8-bit activation blocks the RMSNorm publishes store, per block of 32 codes, a scale and a scaled sum (scale times the sum of the codes), both as f16. The scale fits up to an activation of about 8.3e6. The sum does not: a block of same-signed activations that averages above 2047 puts it past 65504 and it is stored as
inf.mul_mat_q5_k_q8_plane_wmmahas rebuilt such a sum from the codes since engine ggml-org#1962. Every other reader of the stored sum goes through one function,@qwen3_moe_q4k_q8_1_dot(the one-token routed gate/up kernels and the batched decode gate/up), and that one used the stored value as is, so the minimum-correction term becameinfand the whole output with it.Fix
@qwen3_moe_q4k_q8_1_dotchecks the stored sum; if it is not finite it usesscale * sum(the 16 codes this call already holds), which is that half-block's exact share. A finite stored sum takes the path it always took.Proof (gfx1151, one gpu-hold ticket, before = same tree without the kernel change)
New cases
decode_routed_moe.q4_K.large_activation.{1,1000,3000,30000}: the one-token routed decode graph (RMSNorm, router top-8, Q4_K gate/up, SwiGLU, down, weighted sum, next RMSNorm) against a CPU graph on the same weights dequantised to f32. The plan in every case isqwen3_moe_rmsnorm_f32_quantize_q8_1_x4,qwen3_moe_router_projection_top8_fused_decode_f32,qwen3_moe_routed_gate_up_swiglu_q4k_q8_1_x4_next_q8,ggml_mul_mat_id_decode_f32_wave64, ...The two rows below the range are bit-for-bit the same number before and after. These arms were built on #317's head; this branch is the same two-file change on
1bit/hrx-vulkan-patched@0494dfa28.hrx-backend-test(host) at this head: 75 passed, 0 failed.The full
test-hrx-opsdevice suite at this head is queued (ticketcl-hrxkern-2186-suite); I will post its result and the ready line when it has run.🤖 Generated with Claude Code