Correctness: repeatable output on gfx1151 (freed-cell zeroing, deterministic top-k), plus two upstream picks - #20
Merged
Conversation
This reverts the Vulkan part of "Halo/vulkan topk radix (#17)" (ca062e9), which ported the EngramHalo HIP radix kernel as topk_radix.comp, so that the next commit can carry upstream's top_k radix select (ggml-org#28032) unchanged. One implementation of the op, the one upstream settled on, keeps this tree closest to upstream and lets the deterministic slot assignment that follows apply as-is. The two shaders solve the same problem; the difference that matters for Qwen 3.8 Flash-Next is order: topk_radix.comp assigns output slots with atomicAdd, so the selected indices come out in a scheduling-dependent order, and the sparse attention sums them in that order (see the deterministic-scan commit for the measured effect). Upstream's variant also carries the QSA indexer fusion. Co-authored-by: Claude (Opus 5)
…l-org#28032) * vulkan: add top-k radix sort shader for k >= 1024 * add Qwen 3.8 Flash Next top-k tests * add top-k qsa fusion * clean up code (cherry picked from commit daef7b6)
The radix select (ggml-org#28032) assigned output slots with atomicAdd, so identical runs emitted the selected indices in a scheduling-dependent order and, at the tie boundary, could drop a different tied element. On Qwen 3.8 Flash-Next the sparse attention sums the selection in that order, the ULP noise cascades through the QSA layers, and identical greedy requests above 2051 prompt tokens diverge (32k prompt x4: 2 to 3 distinct on every unfixed build, upstream included). Replace both atomicAdd slot counters with a per-chunk subgroup-ballot exclusive scan, walking the columns in ascending index order: the output order is fixed (ascending, which also helps gather locality) and the tie fill keeps the lowest-indexed tied elements. The histogram atomicAdd is unchanged. Verified on gfx1151: test-backend-ops TOP_K 453/453; 32k x4 byte-identical (was 2 to 3 distinct); 1024 x16 byte-identical; layer 3 and layer 47 top-k dumps identical in set and order across runs; prefill 337.5 vs 339.6 t/s and decode 26.3 vs 26.2 t/s at 16k depth (within noise). Co-authored-by: Claude (Opus 5) (cherry picked from commit fe6620c88c599b2f3a6e5ff4a6b99bb4773be38e)
The GDN q/k normalization is defined by flash-linear-attention as
l2norm(x) = x * rsqrt(sum(x*x) + eps)
with eps inside the root. Every GDN call site in the tree uses ggml_l2_norm
instead, which is x / max(sqrt(sum(x*x)), eps), i.e.
torch.nn.functional.normalize - its CUDA kernel cites that page.
The clamp never engages at these magnitudes, so in practice llama.cpp
normalizes with no epsilon at all where the reference has one inside the
root.
transformers made the same substitution when it first added Qwen3-Next and
corrected it three days later in huggingface/transformers#40842, 'Fix the
misalignment between the l2norm in GDN of Qwen3-Next and the implementation
in the FLA library'. vLLM and SGLang vendor FLA rather than reimplementing
it, so neither ever had the clamp.
eps keeps coming from the checkpoint, exactly as every call site already
passed it. The references hardcode 1e-6 for this norm; that is a separate
question and the two agree on every GDN checkpoint in the wild.
ggml_l2_norm itself is correct and unchanged, as is rwkv7-base, its original
caller, which passes normalize's own default eps of 1e-12.
No new ggml op: rms_norm already carries eps inside the root, so
rms_norm(x, eps/n) * (1/sqrt(n)) is exactly x * rsqrt(sum(x*x) + eps).
(cherry picked from commit 0dd72fd)
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com> (cherry picked from commit fe46328)
On gfx11 (RDNA3) WMMA, x + (-0.0) is not exact in f16, so a fully masked flash-attention column with P == +0.0 still leaks the sign of whatever V the cell last held into the accumulator: the output of a request depends on what the previous request left in the cache. On a 1024-token prompt with 16 identical greedy requests, Qwen2.5-7B (f16 KV) alternates between two continuations and Qwen3.8 Flash-Next gives 5-6 distinct ones. Rather than patching the shader (which costs 8-18% dense prefill at depth on gfx1151 by its mere presence in flash_attn_cm1.comp), keep the invariant that a free cell is always zero: the KV buffers are cleared at construction, and every cell that becomes free is memset again, in every layer, when it is freed (seq_rm, seq_keep; clear() wipes up to a per-stream high-water mark, and stream copies mirror the mark). Caches that mirror another cache's cells (the Flash-Next indexer) register with the owner and are zeroed with it. On UMA the Vulkan backend's tensor memset is a host memset; it happens once per free, never per token, and never as a graph node, so graph reuse cannot replay it. Not covered: masked-out cells that belong to another sequence in a unified multi-sequence cache; only a shader-side fix covers that case. Gate (16 identical greedy requests, 1024-token prompt, 129 tokens, per-position top-8 logprob streams compared): 16/16 identical; the fresh-server output is unchanged byte for byte. Co-authored-by: Claude (Opus 5)
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
3 participants
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.
Stacked on #19 (base is its head branch so the diff shows only these six commits); retarget to
masteronce #19 merges.Two output-repeatability defects on gfx1151 Vulkan, each fixed on our fork's v0.7.4 staging branch and validated here on top of
master, plus the two upstream commits they depend on or that were queued for this tree.1. Output depended on stale KV cache cells (all models)
In the cm1 flash attention kernel a fully masked column has P = +0.0, but P times V takes V's sign, and gfx11 WMMA does not add -0.0 exactly. Cells beyond the written extent (the padded n_kv window) hold whatever the previous request left there, so identical greedy requests returned different continuations. This is what lhl reported for Flash-Next (16 distinct continuations in 16 greedy requests on a 1024-token prompt); it is not Flash-Next specific (Qwen2.5-7B with f16 KV alternates between two continuations) and it is present on upstream master.
Fix (
kv-cache: zero freed cells...): the KV cache keeps every free cell at zero. Cells are zeroed in every layer at the moment they are freed (seq_rm, seq_keep, clear up to a per-stream high-water mark; caches that mirror another cache's cells, such as the Flash-Next indexer, are zeroed with their owner). On UMA the Vulkan tensor memset is a host memset, once per free, never per token, no graph nodes. No shader change.Not covered: masked-out cells that belong to another sequence in a unified multi-sequence cache (
--kv-unifiedwith more than one slot). A shader-side fix exists for that case; it costs 8 to 18% dense prefill at depth on gfx1151 by its presence in the kernel, so it is not included.2. Top-k radix select order race (Flash-Next above 2051 prompt tokens)
Upstream ggml-org#28032 (picked here as the first commit) assigns output slots with atomicAdd, so the selected indices come out in a scheduling-dependent order and a tie at the boundary can drop a different element run to run. The sparse attention sums in that order and the noise cascades through the QSA layers: a 32k prompt gave 2 to 3 distinct continuations in 4 identical requests on every unfixed build, upstream included.
Fix (
vulkan: deterministic slot assignment...): both slot counters become a per-chunk subgroup-ballot exclusive scan in ascending index order; ties keep the lowest indices. test-backend-ops TOP_K 453/453. Reported upstream separately.3. Upstream picks
models: use flash-linear-attention's l2norm for gated delta net q/kand its header follow-up (Daniel Han, Georgi Gerganov): the GDN q/k normalization now matches the FLA reference (eps inside the root). Included because this branch carries the Qwen3.6/3.8 delta-net paths.Validation (gfx1151, RADV, this branch on #19's head fa25724)
Repeat gate: identical greedy requests into one llama-server, token streams and per-position top-8 logprob streams compared.
llama-bench, 3 repetitions, branch and #19's head run back to back:
Parity on every cell: upstream's radix select and the parent's run at the same speed, so the difference this PR makes is determinism (the 32k gate above) and alignment with upstream, not throughput. Against current master, which has no radix top-k yet, the same branch measured Flash-Next pp2048 +43% at depth 0 and +77% at 16k, with decode +23% at 16k. The dense cells show the freed-cell zeroing costs nothing. The larger Flash-Next gains from the rest of our Vulkan stack live in PR #17 and the staging branch; this PR is the correctness subset.
Not included (follows the upstream merge)
Five performance commits from our fork are not in this PR because they conflict with this branch's qwen4exp and mmap state and belong on the merged tree: perf flags on by default (0b29b30), TENSOR_READ_LAZY / random-access mmap prefetch for the Flash-Next gather table (77362a8, ad914eb), qwen4exp decode graph reuse with host-side PLE gather (631b9ff), GQA gather-compact for the QSA top-k FA (1c79fe9), last-layer output-row trim (d0006ef, 07050e0). Also pending: the ggml-org#27941 sequence-copy/block-keying fixes (3db62ba) and the FA dequant-guard layout fix. List with measurements: record/perf-commits-status.md on our side.
Relationship to #19
This branch is built on #19's head, so it merges after #19. #19 brings the parent repo's own radix top-k (
topk_radix.comp, ported from the EngramHalo HIP kernel). The first commit here removes that implementation and the second carries upstream's ggml-org#28032 unchanged, so the tree keeps one radix select for the op, the one upstream settled on. Two reasons for that choice: the parent's shader assigns output slots with atomicAdd, the same scheduling-dependent order that made Flash-Next diverge above 2051 prompt tokens on upstream before the deterministic scan in this PR, and ggml-org#28032 carries the QSA indexer fusion upstream master uses. Least divergence from upstream was the deciding factor.Credit
lhl for the report that started this (16 distinct continuations in 16 greedy requests on a 1024-token prompt). Ruben Ortlam for ggml-org#28032. Daniel Han and Georgi Gerganov for the l2norm correction.
Disclosure: analysis and patches prepared with AI assistance (Claude), reviewed and tested by me on hardware.
🤖 Generated with Claude Code