Repository navigation
hrx: a fused dispatch may not swallow a value the caller asked to read back (engine #1078) - #209
Conversation
…d back (engine ggml-org#1078) The zha q/k dump (src/llama-zha-dump.cpp, #179) marks the tensors it wants with ggml_set_output() and reads them back after graph_compute. On the HRX route its pre-RoPE half came back as zeros - every layer, every row - while the post-RoPE half was fine: -dev none : L0 qpre max 10.41, rms 1.46 (values) -dev HRX0 -ngl 99 : L0 qpre every value 0.0 (zero-filled buffer) -ngl 99 DISABLE=fused: L0 qpre rms 14.66 (kpre), values back (the fusion is the difference) Cause: ggml-hrx treats a GGML_TENSOR_FLAG_OUTPUT value as external, so a fused match writes its *root*'s output for the caller (graph.cpp:tensor_is_external), but nothing does the same for the nodes a match *covered*: their only copy stays inside the fused program and their buffer is never written. So a match is now declined when it would cover a node whose tensor carries the read-back marker, and the plan falls through to a narrower registration that writes the node itself (one GGML_HRX_LOG_DISPATCH line per decline, 8 then a suppression notice). The marker is the explicit name prefix llama_zha_dump applies ("zha-readback.<name>"), not the bare GGML_TENSOR_FLAG_OUTPUT: llama.cpp marks several tensors as outputs for API reasons (result_norm is t_embd for the embeddings API), and honouring the flag alone declined the final-norm fusion in every ordinary run - measurable in the plan, and not something to trade for a debug tool. With the prefix, nothing outside a dump run carries it. Measured on the pin (Qwen3-4B-IQ4_XS, -dev HRX0 -ngl 99, one gpu-hold ticket per arm set, same prompt): ordinary run : 0 declines, first-token logprob -1.3745098, top-5 identical to 6 dp to the build without this change - i.e. the plan is the one it had before dump run : 8 declines naming zha-readback.Qcur_normed-N / Kcur_normed-N, qpre L0 rms 1.5323 and kpre 14.6636 - the GGML_HRX_DISABLE_DISPATCH=fused reference reads 1.5322 / 14.6648, so these are the real values and not a different approximation - and no "unsupported HRX node" A dumped intermediate with no fallback registration now fails loudly instead of writing zeros; that is the trade a debug tool should make. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 154b0de4 — the fix is precisely scoped, and the reasoning that scoped it is in the code.
What I checked, and the part that decided it. The naive fix for "a fused dispatch swallowed a value the caller reads back"
is "decline any match covering a tensor flagged as an output" — and the comment says why that is wrong, from measurement:
// The marker is the explicit name prefix llama_zha_dump applies ("zha-readback."), not GGML_TENSOR_FLAG_OUTPUT on its
// own: llama.cpp marks several tensors as outputs for API reasons (result_norm is t_embd), and honouring the bare flag
// declined the final norm fusion in every ordinary run.So the predicate needs both the flag and the prefix, the root is exempt (a match always writes its root's output), and a
match that covers a readback node is declined so it falls through to a narrower registration that writes the node. That is
the correct shape: the cost is paid only by a run that actually uses the dump, and no ordinary run is planned differently —
which is the property that makes this safe to land without a device.
Checked in the loop rather than assumed: covered_nodes entries are bounds-checked against nodes.size(), the root index is
skipped, and the value lookup is null-checked before the predicate sees it.
The evidence in the body is the right instrument for this bug: pre-RoPE q/k came back all zeros on the HRX route, the
post-RoPE half was fine, and GGML_HRX_DISABLE_DISPATCH=fused brought the values back — a symptom, a localisation and a
control, which is what a placement bug needs.
Not verified by me: the device run (GPU held). The src/llama-zha-dump.cpp +14 half — the dump marking its own tensors with
the prefix — is what makes the predicate fire; that is a one-line invariant worth keeping in mind if the dump is ever renamed.
What this fixes
The zha q/k dump (
src/llama-zha-dump.cpp, #179) marks the tensors it wants withggml_set_output()and reads them back aftergraph_compute. On the HRX route its pre-RoPE half came back as zeros — every layer, every row — while the post-RoPE half was fine:Cause, in ggml-hrx:
graph.cpp:tensor_is_external()treats aGGML_TENSOR_FLAG_OUTPUTvalue as external, so a fused match writes its root's output for the caller — but nothing does the same for the nodes a match covers. Their only copy stays inside the fused program and their buffer is never written, so a read-back afterwards sees the untouched (zero) buffer.The change
A match is declined when it would cover a node whose tensor carries the read-back marker, and the plan falls through to a narrower registration that writes the node itself. One
GGML_HRX_LOG_DISPATCH=1line per decline, then a suppression notice after 8.The marker is the explicit name prefix
llama_zha_dumpapplies (zha-readback.<name>), not the bareGGML_TENSOR_FLAG_OUTPUT. My first version keyed on the flag alone, and it declinedcommon.rmsnorm_binary_f32in every ordinary run: llama.cpp marks several tensors as outputs for API reasons (result_normist_embdfor the embeddings API), and the final-norm fusion was being dropped to serve a debug tool. With the prefix, nothing outside a dump run carries the marker, so an ordinary run declines nothing and is planned exactly as it was — a property stated from the code and checked below, rather than argued from a benchmark. (I did try to price the flag-only version: logits were bit-identical, but the box'sllama-benchspread on the same binary was±147 t/son a 1119 t/s prefill, which cannot resolve a one-op difference and is exactly why I did not want the change there at all.)Measured
Qwen3-4B-IQ4_XS,
-dev HRX0 -ngl 99 -fa auto, same prompt, onegpu-holdticket per arm set, f16 dumps:qpreL0 (zero_frac, rms)kpreL0LLAMA_ZHA_DUMP_DIR)" The"−1.3745098, top-5 identical to 6 dp vs the build without this changezha-readback.Qcur_normed-N/Kcur_normed-N" The"−1.3745098GGML_HRX_DISABLE_DISPATCH=fusedThe dumped values match the fusion-disabled reference to 4 decimal places, so this is the real value and not a different approximation; no arm produced
unsupported HRX nodeor a compute error.One deliberate trade: a dumped intermediate that has no fallback registration now fails loudly instead of writing zeros. That is what a debug tool should do.
Files:
ggml/src/ggml-hrx/dispatch_registration/dispatch-registry.cpp(edits in place, their header kept) andsrc/llama-zha-dump.cpp(ours); 2 files, +92/−2.Refs engine ggml-org#1078 (where the zero read-back was found), fork #179 (the dump itself). No merge from me.