CUDA: re-capture the graph when a CPY node's read-source address changes#2136
Open
joelfarthing wants to merge 1 commit into
Open
CUDA: re-capture the graph when a CPY node's read-source address changes#2136joelfarthing wants to merge 1 commit into
joelfarthing wants to merge 1 commit into
Conversation
ggml_graph_node_has_matching_properties exempted every GGML_OP_CPY node from the source-address check, for both operands. The exemption is needed for KV-cache write copies, whose destination advances each step through the indirect-destination path. It also skips a CPY whose read source (src[0]) moves between graph replays while the surrounding subgraph stays shape-stable, so the captured kernel replays against a stale source pointer and reads the previous step's bytes. Keep the exemption for the destination operand (src[1]) only, and compare a CPY's read source (src[0]) like any other node. Stable-source copies are unaffected and force no additional re-captures; a CPY whose read source genuinely moves now re-captures instead of reading stale memory.
Owner
|
I need to be away for a few days and don't like the idea of merging this PR just before that. Will merge when I'm back. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I came across this while experimenting with moving openPangu attention off its bespoke path onto the shared DSA gather (#2109) and indexer-mask (#2119) machinery. The fix is general to any CPY whose read source moves under CUDA graphs and stands on its own.
In
ggml_graph_node_has_matching_properties, aGGML_OP_CPYis exempt from re-capture when its source address changes, for both operands. That is meant for KV-cache writes: their destination (src[1]) advances each token but is repaired at replay through thecpy_dest_ptrsindirection table.src[0]is the read source and has no such indirection, so a CPY whose source moves while its subgraph stays shape-stable never re-captures and replays a stale pointer. The fix keepssrc[1]exempt and comparessrc[0]like any other source:Where it applies: openPangu-2.0-Flash, q8_0 latent cache,
-fa off, greedy. Past ~600 tokens the sliding-window latent-K read (a Q8_0-to-F32 cast whose q8_0 view steps to a new block) replays the previous block's bytes, so attention reads stale K and output collapses into a repetition loop; with the fix it tracks the window and stays coherent.The narrowing is safe: every other CPY reads from a stable address, and a KV write keeps
src[0]at a stable compute-buffer address, so this adds no re-captures for them. On Qwen2.5-0.5B (no moving-source CPY) the graph counters are identical patched vs unpatched (33 captures, 0 disabled; balanced 3-rep A/B) and decode throughput matches within noise.Correctness, greedy
--temp 0 -n 1000, unpatched main vs PR patched by swapping onlylibggml.soon the posted base: unpatched runs to the token cap in a 23x-repeated loop, patched stays coherent and stops on its own. The two arms emit token-identical code until they diverge. At the window step, the stale K read makes attention re-emit the previous line instead of advancing.Current main collapses:
With this PR, generation continues:
Throughput, warm A-B-A, openPangu Q4_K_M,
-ngl 999 -ot exps=CPU -ub 2048 -fa off, swapping onlylibggml.so. Decode (S_TG) is the affected path, patched vs unpatched:Post-onset decode delta averages +0.06%, prefill +0.02%, both within noise. Instrumented counters over a 915-token decode: 1413 captures, peak 3 consecutive updates, 0 graphs disabled, so graphs stay engaged. (The fallback disables a graph at 4 consecutive updates; the q8 window steps periodically and stays under it, so correctness holds and the added re-captures cost nothing measurable.)
Reproducer: openPangu-2.0-Flash Q4_K_M, RTX 4070, CUDA 13.3, Release. Prompt "Write a quicksort implementation in python", greedy:
Separately, I did spot a real problem worth its own fix: the Q8_0-to-Q8_0 transpose used by MLA
mla=2q8 caches is admitted into CUDA graphs but writes its captured destination directly and ignores the indirection table, so a moving destination there would replay stale. I'm calling it out of scope for this PR; openPangu never hits it (it dequants each q8 boundary to F32 first, tripwire-confirmed), and I would rather keep this change to two lines and address that path on its own.