mtmd/ggml: add ggml_build_forward_order - #26649
Conversation
ggml_build_forward_expand marks the tensor and all its ancestors for compute, so using it as a pure ordering hint (keeping q, k and v together) defeats ggml_build_forward_select: the unselected branch is forced to run with inputs that were never uploaded. In the mtmd audio graph this makes GEN_WAV calls execute the GEN_CODE branch with a stale inp_code0, hitting the get_rows bound assert on CPU. Add ggml_build_forward_order, which inserts nodes without the compute flag; the flag is restored when the branch is actually selected. Switch the q/k/v hints in clip_graph::build_attn to it.
github-actions
Bot
added
ggml
ServeurpersoCom
changed the title
ngxson
left a comment
ngxson
left a comment
There was a problem hiding this comment.
tested & confirmed not breaking existing use case
|
This breaks |
Yes. ggml_build_forward_order will land in the standalone GGML repo on the next llama.cpp to GGML sync from ggerganov, so the formula just needs to wait for a GGML release that includes it. On your side you can also pin your own GGML fork in the formula and cherry-pick the GGML commits straight from llama.cpp |
Hm, this sounds incorrect. The expansion stops when it reaches a visited node marked for computing. Plus the pattern is already used in I think the root cause might be different. |
|
Right, my summary in the PR description was ambiguous: "defeats ggml_build_forward_select" reads as if the select mechanism itself was broken, which it isn't, the visited-node early-out works as intended. The actual issue is ordering: ggml_visit_parents_graph sets the COMPUTE flag on first visit and recurses with the same compute value, so the q/k/v hints in build_attn mark their whole ancestor chain while the gen graph is still being built, before the selects at the end of build() run, and the compute=false pass never clears flags. So during a GEN_WAV call the GEN_CODE chain stays flagged up to the last attn hint, including the get_rows on inp_code0 which is never uploaded on that path. libllama never hits this because it is safe by construction: build_inp_embd selects before any hint exists, and build_sampling feeds inactive branches a valid dummy row. |
Do you mean that the |
|
No, everything happens inside build(): the GEN_CODE branch is constructed first via cg.prefill()/cg.step(), which call build_attn() for each layer, and that's where the q/k/v expand hints used to mark the chain. The ggml_build_forward_select() calls only come at the very end of the same build(), so by the time the compute=false pass visits out_codes, the chain is already flagged and nothing clears it. |
|
Any workaround on the model side would be costly: uploading valid dummy inputs means running the whole predictor on every GEN_WAV call just to discard the result, splitting back into two graphs loses the static topology from #26254, and dropping the expand hints degrades node ordering for every model going through build_attn. That's what justifies fixing it in ggml: a pure ordering hint was the missing primitive, and it costs nothing at runtime. On my side I also tried fusing the two graphs in my standalone qwentts.cpp and hit exactly this issue, but since the fused design is inherently single batch I preferred to keep my separate per-stage graphs and multi-batch path, it's a micro project dedicated to one model so batching matters more to me than the static topology. On top of that, this mono model micro project turned out to be a proof that GGML can compete with and even outperform PyTorch at scale, it now powers a cloud voice assistant on a HF space |
Homebrew can wait, but I’d like to clarify that I see this as a release-sequencing issue. |
|
@ServeurpersoCom Ok, got it - the change is good. We should actually replace all |
|
@p-linnane Thanks for flagging. We'll work to guarantee system-ggml compatibility of the llama.cpp builds for downstreams. To do that, there will be a new |
* ggml: add ggml_build_forward_order ggml_build_forward_expand marks the tensor and all its ancestors for compute, so using it as a pure ordering hint (keeping q, k and v together) defeats ggml_build_forward_select: the unselected branch is forced to run with inputs that were never uploaded. In the mtmd audio graph this makes GEN_WAV calls execute the GEN_CODE branch with a stale inp_code0, hitting the get_rows bound assert on CPU. Add ggml_build_forward_order, which inserts nodes without the compute flag; the flag is restored when the branch is actually selected. Switch the q/k/v hints in clip_graph::build_attn to it. * nit: reduce comments (AGENTS.md)
* ggml: add ggml_build_forward_order ggml_build_forward_expand marks the tensor and all its ancestors for compute, so using it as a pure ordering hint (keeping q, k and v together) defeats ggml_build_forward_select: the unselected branch is forced to run with inputs that were never uploaded. In the mtmd audio graph this makes GEN_WAV calls execute the GEN_CODE branch with a stale inp_code0, hitting the get_rows bound assert on CPU. Add ggml_build_forward_order, which inserts nodes without the compute flag; the flag is restored when the branch is actually selected. Switch the q/k/v hints in clip_graph::build_attn to it. * nit: reduce comments (AGENTS.md)
* ggml: add ggml_build_forward_order ggml_build_forward_expand marks the tensor and all its ancestors for compute, so using it as a pure ordering hint (keeping q, k and v together) defeats ggml_build_forward_select: the unselected branch is forced to run with inputs that were never uploaded. In the mtmd audio graph this makes GEN_WAV calls execute the GEN_CODE branch with a stale inp_code0, hitting the get_rows bound assert on CPU. Add ggml_build_forward_order, which inserts nodes without the compute flag; the flag is restored when the branch is actually selected. Switch the q/k/v hints in clip_graph::build_attn to it. * nit: reduce comments (AGENTS.md)
* ggml: add ggml_build_forward_order ggml_build_forward_expand marks the tensor and all its ancestors for compute, so using it as a pure ordering hint (keeping q, k and v together) defeats ggml_build_forward_select: the unselected branch is forced to run with inputs that were never uploaded. In the mtmd audio graph this makes GEN_WAV calls execute the GEN_CODE branch with a stale inp_code0, hitting the get_rows bound assert on CPU. Add ggml_build_forward_order, which inserts nodes without the compute flag; the flag is restored when the branch is actually selected. Switch the q/k/v hints in clip_graph::build_attn to it. * nit: reduce comments (AGENTS.md)
4 participants
Overview
Follow-up / CPU Fixe for Qwen3-TTS PR #26254
The plumbing was already there, ggml_build_forward_impl takes a compute flag, it just had no public entry point, so build_attn had nothing but expand to reach for.
Tested on my side and confirmed by @woheller69 on the CPU path that triggered the assert.
Additional information
ggml_build_forward_expand does two things at once, it places a node in the graph and it marks that node and all its ancestors for compute, so using it as a pure ordering hint defeats ggml_build_forward_select and the unselected branch runs anyway, on inputs that were never uploaded. That is the crash reported on #26254: during a GEN_WAV call the whole GEN_CODE branch is still computed and reads a stale inp_code0, which trips the get_rows bound assert on CPU while CUDA silently reads an out of range row, so it only shows up when the mtmd gen graph runs on the CPU backend. This adds ggml_build_forward_order, which only does the placement, and switches the q/k/v hints in build_attn to it.
Requirements