indexer_topk: fix quantized q8_1 scratch sizing on CUDA - #2158
Conversation
The quantized-K path under-sized its q8_1 scratch buffer: it allocated q->ne[1]*max_rows blocks using the unpadded head dim, but quantize_mmq_q8_1_cuda writes q_padded/QK8_1 blocks per row and must process all q->ne[1]*nrows rows. Size the buffer by (q_padded/QK8_1) blocks x (q->ne[1]*max_rows) rows and pass the full q->ne[1]*nrows row count so the scratch cannot be overrun and every query row is quantized. CUDA graph identity: INDEXER_TOPK dispatches a source-type-specialized kernel (dense F16 vs quantized cache, F32 vs F16 mask). Source addresses alone do not identify the captured kernel, so snapshot each source type in ggml_graph_node_properties and force re-capture when an INDEXER_TOPK source type changes, preventing a reused graph from replaying the wrong kernel variant when sources are reallocated at the same address. CPU backend: report INDEXER_TOPK support via iqk_indexer_topk_supported (guarded by GGML_USE_IQK_MULMAT) so the scheduler places the node on a backend that can run it. Harden the scheduler's pass-5 node-assignment check from assert to GGML_ASSERT so a node no backend supports fails as a defined abort under NDEBUG instead of indexing sched->backends[-1].
| } | ||
|
|
||
| namespace { | ||
| bool iqk_checked_mul_size(size_t a, size_t b, size_t & result) { |
There was a problem hiding this comment.
This sort of thing can only be written by an LLM. Let me see: suppose we handle correctly size_t overflow. Do we expect that a situation where we are dealing with number if operations greater than the maximum value of size_t on a 64-bit platform to ever complete? On a 10 TFlop CPU this will take several days.
ikawrakow
left a comment
There was a problem hiding this comment.
Please discard all changes to IQK. If we cannot compute the indexer topk op on the CPU, then it shall not be used instead of having 160 LOC of checks.
Done. Thanks again. I also added a fallback when unsupported; please let me know if that's not wanted. |
| // INDEXER_TOPK dispatches a source-type-specialized kernel (dense F16 vs quantized cache, | ||
| // and an F32 vs F16 mask variant). Source addresses alone do not identify the captured | ||
| // kernel, so a reused graph whose sources were reallocated at the same addresses with a | ||
| // different type would replay the wrong kernel. Force re-capture when a source type changes. |
There was a problem hiding this comment.
You actually ran into this situation? Can you share how?
There was a problem hiding this comment.
You actually ran into this situation? Can you share how?
Yes, but this came from a focused INDEXER_TOPK test harness, not from changing cache types inside one openPangu context.
The harness creates one CUDA backend and then runs three same-shape fixtures sequentially: F16 K with an F32 mask, F16 K with an F16 mask, then Q8_0 K with an F32 mask. Each fixture frees its backend buffer before the next one, but the CUDA backend and its graph cache remain alive. The allocator reused the same source addresses.
With CUDA graphs enabled, the matcher accepted the later graph because the output shape/strides, source addresses, and op parameters matched. It did not see that the source types had changed, so INDEXER_TOPK replayed the previously captured type-specialized kernel. The numeric reference/parity check failed. The same sequence passed with CUDA graphs disabled.
Recording the source types forced a recapture and made the graphs-enabled sequence pass. So I did hit the exact same-address/different-type case, but in a deliberate same-process test. I should have made that distinction clear in the description.
There was a problem hiding this comment.
OK, theoretically the graph capture/comparison logic can fail in gazillions of different ways. If we wanted to guard against all possible failure modes, but at the same time still be able to reuse captured graphs, it would become way more complicated and possibly so expensive that it wouldn't bring any benefits to use it. But fortunately for us, we are not writing airplane control software here, so we go by actual usage in the inference engine only.
openPangu-2.0-Flash drives the shared
GGML_OP_INDEXER_TOPKop with a quantized indexer K cache (-ictk q8_0), which comes across a scratch-sizing bug in the op's CUDA path. The bug lives in code every INDEXER_TOPK caller shares (DeepSeek2, GLM-DSA), so I'm presenting the fix here separately from the openPangu operator PR #2159, which stacks on it.The quantized path under-sizes its q8_1 scratch. The old size expression omits the per-row block count
q_padded / QK8_1thatquantize_mmq_q8_1_cudaactually writes, and the call passesnrowswhere the quantizer must process allq->ne[1] * nrowsrows, so with the multi-head indexer q the scratch is under-allocated and the matmul consumes rows that were never quantized. The fix sizes the buffer at(q_padded / QK8_1)blocks by(q->ne[1] * max_rows)rows and passes the full row count to the quantizer.The rest of the diff is the not-used-when-unsupported behavior from the review. The graph builders now construct the fused top-k node only when a loaded backend reports support for it, and otherwise keep the unfused chain, via a small
supports_ophelper on the build context. The CPU backend reports support underGGML_USE_IQK_MULMATbuilds with a blanket true, which is exactly base'sdefault: return true, and reports false without IQK. So in every IQK build (the default) node placement and graphs are unchanged from base; the only build whose graph changes is a non-IQK CPU-only build, which on base claimed the op and then could not run it, and now keeps the unfused chain instead.Scope and blast radius. The CUDA support gate is unchanged:
ggml_backend_cuda_supports_opstill returns true for INDEXER_TOPK unconditionally.build_deepseek2.cppproduces the same graph as base in any build where a backend supports the op, which is every real configuration. The op's numerical contract is untouched.Validation, measured at the head this PR opened with (
77ab94c4) against main (9d07d8681, still the current base); the fix hunk is byte-identical at the present head and nothing added since touches the quantized path. Config:-ngl 999 -ot exps=CPU -fa off -ctk q8_0 -ictk q8_0 -fidx -dsatk 64, greedy.A 32K-token needle (planted near 65% depth) does not survive a quantized indexer cache on main: raw completion degenerates to
0 = 0 = 0 =, and through the chat template the model concludes the passage contains no mention of any such code and collapses. With this fix the raw-completion needle is retrieved verbatim.Perplexity over wikitext (n_ctx 512, batch matched so single-sequence perplexity packs one chunk per batch) pins the failure to the quantized path:
(19 chunks; the full 109-chunk q8_0 run is 14.91 on main and 8.31 with the fix.) The degradation is specific to a q8_0 indexer cache on main: every other cell sits near 9.8, including main's own f16, so the true perplexity is about 9.8 and the fix restores the quantized path to it. The f16 result is byte-identical between the two (9.8953 both), so the scratch change touches only the quantized path.
-ictkdefaults to f16.For GLM-DSA or DeepSeek v4, the A/B to run is
-fidxon CUDA: F16 indexer cache, unchanged selections; q8_0 indexer cache, corrected selections.