Repository navigation
fix: the code review's findings across the server, the k-pool, sparse attention, the MoE kernels and the graph backend - #90
Merged
Merged
Conversation
…rns, before the chat parser reads it utf8_replace_malformed kept any sequence whose bytes were shaped like UTF-8: a surrogate (ED A0 80), an overlong form (C0 80, E0 80 80, F0 80 80 80), a code point past U+10FFFF (F4 90 80 80) and a lead that leads nothing (F5-FF) all passed as well-formed. The chat parser's strict JSON dump then threw on them inside a tool call's string argument (Qwen3-Coder's <parameter=...>), and the request answered 500, or a stream ended on an error event: the failure b915c5e was written to remove, for the bytes it did not cover. The second byte of a lead is now checked against its range (Unicode Table 3-7: E0 A0-BF, ED 80-9F, F0 90-BF, F4 80-8F, the rest 80-BF; C0, C1 and F5-FF lead nothing), and a malformed sequence is one U+FFFD for its maximal subpart, as the JSON writer shows it (E4 B8 41 was two, now one). An incomplete tail is held back only while every byte of it is right so far. test-utf8-replace: 18 cases, 10 of which fail on the function before this change (every one the pattern check let through, the maximal subpart, and E0 80 / ED A0 held as a tail).
…nd a ubatch clears only its own kpool_dirty was one flag for the cache. A seq_add that regroups pools (a shift that is not a whole number of pools) set it, the next ubatch rebuilt, re-emitting the pooled keys of its own sequences only, and the flag was cleared. A shifted sequence absent from that ubatch kept its stale pooled keys, and its indexer scored the wrong pools: seq_add(seq 0, -5), a decode of seq 1, a decode of seq 0. llama_kpool_stale holds the marks as a bitset of sequences: seq_add and seq_div mark the sequence they move (every sequence for seq_id < 0), a ubatch rebuilds when it holds a marked sequence, and its set_input clears that ubatch's sequences. The shift test moves from llama_memory_hybrid::seq_add into llama_kpool_stale::shift_regroups, unchanged. test-kpool-input runs that sequence with the indexer cache kept as set_rows writes it: 0 stale pools after the shifted sequence's own decode, and 1 under the former single flag, so the check can fail.
…ect path A byte per cell of the cache (512 KB at 524,288 cells) was allocated and zeroed for every stream of every decode before the view path, which never reads it, returned. It is now made after the view path, where the direct path reads it. No byte of an input changes: test-kpool-input builds the inputs from the views and from the cells and compares them, ok.
…_0 gets its FFN chain case mmvq_moe_quantize_y's loop ran while j0 < total, and its shuffles name all 32 lanes: a total that is not a multiple of 32 would split a warp, and the lanes still in would wait on the ones that left. Today no graph reaches that, since ggml_cuda_mmvq_moe_y_bytes admits only a copy of whole 16-byte units, which holds a quantized row to a multiple of 128 columns and so total to a multiple of 32. The bound is now rounded up to a whole warp, so the kernel holds without leaning on that rule; for every shape the ring takes today it is the same bound. test-backend-ops: the routed FFN chain in Q8_0 at 2 and 3 tokens, the shapes where the ring takes Q8_0 and the down quantizes its own vectors. The chain had cases for IQ3_XXS only.
…arp-whole bound (466d8d4)
…6 pools, not all at once After a position mutation the rebuild re-emits every pool (n_new_max = n_pools, 131,074 at 524,288 cells), and the compression gathered every pool's members and made their softmax at once: about 1.07 GB of compute buffer at d_idx 128 that no reserve sized, allocated in the middle of serving. llama_kpool_compress now makes the pools 4,096 at a time, each chunk's write expanded into the graph before the next chunk is built, so the allocator gives each chunk the last one's memory (about 34 MB). A decode's few pools are one chunk, the graph it always built. Reachability from rig's serve: only context shift and cache reuse call seq_add in llama-server (server-context.cpp:3446 and :3773), both off by default (common.h:600 ctx_shift = false, :657 n_cache_reuse = 0), not set by rig's GLM head, and n_cache_reuse is not a request field (server-schema.cpp:527). So the rebuild is not reached there; this bounds it for any server that turns either on. test-kpool-input compresses 4,096 pools in chunks of 512 and all at once: the same bytes in every pooled row, 1,048,576 bytes of compute buffer against 8,388,608.
…Q2_0 group takes computed nodes only, and four smaller reads - ggml_cuda_check_fusion_memory_ranges let the fused top-k's weights and ids overlap any src at 2-4 rows (5c03ed9), but its block barrier orders only the logits' reads: the selection bias is read past it, while a sibling warp writes. At 2-4 rows the outputs now may overlap a src inside the logits' bytes (the logits, or gpt-oss's reshape of them) and every other src is checked. One row keeps upstream's exception. No shipped model changes: GLM-5.3's bias is a weight (op NONE), which the check never reads. - ggml_cuda_pq2_mma_group_member took a MUL_MAT without GGML_TENSOR_FLAG_COMPUTE (ggml_build_forward_select's other branches), which the node loop skips and a group would compute. It now takes computed, non-empty nodes only, as the q8_1 sharing and the norm-FWHT fold already check theirs. - ggml_cuda_graph_get_key read nodes[0] of an empty graph; its key's first node is now null. - GGML_CUDA_TIME_LAUNCH and GGML_CUDA_COUNT_INFLIGHT read as ggml_env_switch does (=true or =on was off). - test-backend-meta-capture read GGML_META_CAPTURE_LEGACY by presence, so =0 expected no captures where the backend made them and reported FAIL; it now reads it as the backend does.
…computed nodes and the switch reads (458b5c1)
…ail the cells hold, and an overflowing row traps A row of GLM-5-Next's sparse attention is live at its top-k cells and at its tail, the cells of its sequence at positions [(q + 1)/r*r, q]. n_kv_max counted r - 1 tail cells, padded to 32. A sequence holding two cells at one position has a longer tail, and the CUDA compaction kept the first n_kv_max cells of a row with no signal. - llama_kpool_tail_max counts the longest tail of the ubatch in the cells (r - 1 when no sequence of it holds two cells at one position, an O(1) check), and build_inp_kpool stores it as n_tail. build_attn_sparse sizes n_kv_max = llama_kpool_n_kv_max(top-k cells, n_tail), and can_reuse refuses a graph built on another n_tail. - llama_kpool_set_input returns the most tail cells it granted a row, counted where kpool_mask_row writes them, and llm_graph_input_kpool::set_input asserts it is within n_tail. - Both compaction kernels trap on a row with more finite cells than n_kv_max (ggml.h documents it). test-kpool-input: "a tail of 36 cells: 2084 live cells a row, n_kv_max 2080 for an r - 1 tail, 2112 for the longest tail"; in the random rounds 733 builds held a tail over r - 1 cells, each within the bound, and set_input's count equals sel_mask's finite columns on the view and the cell paths. With llama_kpool_tail_max returning r - 1 always, the fixed case and the rounds fail. test-kpool-can-reuse: a longer tail refuses reuse.
pre' G pre is a sum of squares, so dsv4_hc_gram_pre took it as one, but it is summed from G's entries: streams that cancel give a small value whose rounding can land under 0, and rsqrtf of a negative is NaN for the token's whole row. The other two RMS sites sum squares directly and cannot. One fmaxf, in a per-token path, not a per-element one.
… enforces it The chunked KDA kernels stage exp of the gate's prefix sums in fp16 (stage 3's q and k operands, the decay), which holds only for a log-decay at or under 0; a positive gate saturates them to inf, and the only guard was cgdr_to_fp16's debug-only assert, so a Release build served inf silently. The shipped model's gate is lower_bound * sigmoid with lower_bound asserted negative (glm5next.cpp:82), but the op is public. - ggml_gated_delta_net documents g as a log-decay, KDA's at or under 0. - A raw KDA gate whose lower_bound is above 0 is not chunk-eligible, so it runs on the recurrent kernel. - An activated gate above 0 traps in cgdr_kda_fwdsub_intra_kernel, in the barrier Step 0 already had (__syncthreads_or over the block's own loads), so the shipped path takes no extra sync and no extra read. test-backend-ops gains four KDA cases for the chunked path: strided q/k/v (the qwen35 views, no cont anywhere) with activated and with raw gates, and a chunked pass of one chunk alone (128 tokens with K = 113 snapshot slots, so 16 tokens chunked and 112 on the recurrent tail) over one and two sequences.
…which reads live tiles The switch gates shall_use_sparse alone, so with it the flash attention runs the dense kernel as it did before the gather: over the KV steps a row of its Q tile sees (launch_fattn's kv_live, 2f0ca78, which predates the gather and has its own switch). The whole cache under the mask needs GGML_CUDA_FATTN_LIVE_TILES_LEGACY=1 as well. Documented in both places rather than widened: two measured optimizations keep two switches.
…e, so a slot never drafts from another conversation's 4ec3d00 made the kept row carry the position it is for, and h_before pairs a token with it only at that position plus one. A position is not an identity: a slot that ends conversation Z with a row kept for position P, then takes conversation Y out of the prompt cache (--cache-ram) resuming at P + 1, paired Y's first row with Z's h. The MTP drafter implemented neither get_state nor set_state, so the stash a checkpoint carries (server-context.cpp:2855) was always empty for it, and nothing dropped the row when a slot's cells were replaced or removed. The cost is draft acceptance, never the text: a wrong h row only makes the head's drafts worse. The row and its position now travel with the state they belong to. The MTP drafter has get_state and set_state (as the dflash one does), a sequence with no kept row has no state, and an empty state clears it. server_prompt_data carries that blob beside the target's and the draft's, prompt_save fills it, and the cache's load restores it. prompt_clear, which removes the sequence's cells, clears it. Checked: llama-server builds; the prompt cache's own server tests (test_kv_keep_only_active) and the slot, context-shift, speculative and basic suites pass, 36 in all. The MTP path itself has no automated check here, since it needs the private pack; the change is by construction, and LLAMA_MTP_PENDING_H_LEGACY=1 still pairs with the kept row whatever its position.
…ere it dropped an expert's last tiles 6477031 launches the experts' non-empty column tiles back to back and sizes the list at min(n_expert*ceil(n_tokens/J), (ncols_dst + min(n_expert, ncols_dst)*(J - 1))/J). The first term counts an expert at most once for a token, which holds for every graph built here (ids is a top-k of distinct experts) and for nothing else: ids naming an expert twice for one token gives it more than n_tokens columns, and four experts holding 5, 5, 5 and 1 of 16 columns at J = 4 need 7 tiles where the term allows 4. mmq_moe_tiles_kernel wrote what fit and dropped the rest, so those columns' rows of dst were never written and the result was wrong with no sign. The kernel now traps on a list past the bound, and the bound states the precondition it rests on. The shipped path is unchanged: the check is one comparison per expert in the list kernel, which runs once per launch on one block.
…SPARSE_LEGACY's real baseline Rows for 00e9132, 47fb5e6, caaad77, 0ac4349, 433d88e, 598d835 and 60ee437, each with its evidence. 0b0e97b's off-switch column said GGML_CUDA_FATTN_SPARSE_LEGACY=1 gives the whole cache under the mask; it gives the dense kernel, which reads 2f0ca78's live tiles, so it now names GGML_CUDA_FATTN_LIVE_TILES_LEGACY=1 too.
… and the rows for cdd2dff and c56099b MOE_FFN_CHAIN ran at 1, 2, 3 and 5 tokens. At 8 tokens on 8 experts all used, the ring's pair tables are exactly full (64 = MMVQ_MOE_MAX_PAIRS, bit 63 of an expert's pair mask), which MUL_MAT_ID covers for one launch and the chain did not for the gate/up and down on one ids. One case a type.
…royed, since the slots outlive it destroy() resets spec, and the slots keep pointing at it: --sleep destroys the model and the drafter while the slots stay, and load_model builds both again and reassigns slot.spec. Nothing read the stale pointer before cdd2dff, which made prompt_clear speak to the drafter, and a clear between a sleep and a reload would have read freed memory. test_sleep, the prompt cache's own tests and the speculative suite pass, 18 in all.
…er a strict view of them 458b5c1 let a fused topk-moe's output overlap any src CONTAINED in the logits, on the ground that the block's warps read every logit before the barrier they then write past. Containment is wider than that ground. The kernel reads logits->ne[1] rows (topk-moe.cu:367), while the rule is handed ggml_nrows (ne1*ne2*ne3), so for a logits tensor with ne[2] > 1 the un-read tail passes containment; and the bias, which topk-moe.cu:170 loads 40 lines PAST the barrier, would pass it too if it were ever a view of the logits. Neither is reachable in this tree (no graph builds such logits, and every ffn_exp_probs_b is a model weight, so GGML_OP_NONE skips it before the rule runs and its buffer is not the logits'), so this is the rule stated as what it rests on, not a fix for a live defect. The test is now identity: the same buffer, the same data pointer and the same byte count. That is the logits and a reshape of them, which is the case the comment always described, and gpt-oss keeps its fusion. Anything else is refused, which only ever refuses a fusion. tests: every registered topk-moe shape has 1 row or 22, and the one with 4 (160 experts) is refused by ggml_cuda_should_use_topk_moe for not being a power of two, so nothing reached the 2-to-4-row rule at all. A {128, 4, 1, 1} shape at 8 used experts does.
…h the Gram front's clamp (433d88e) needs No existing DSV4_HC case reaches a negative pre' G pre, so removing the clamp passed every one. Here 12 of 16 tokens hold streams cancelling in pairs to within 1e-7 at a scale of 100 under equal pre weights; an f32 replay of the kernel's arithmetic rounds the form under -n_embd*eps_norm for 15 of 40 such tokens, which rsqrtf makes NaN.
… MTP row its dangling-pointer fix c7f9580 narrowed 458b5c1's containment test to identity, and added the first test case that reaches the 2-to-4-row rule at all; ddaaf00 cleared the slot drafter pointer cdd2dff left dangling. Both refine a divergence already listed, so they join its row rather than opening one.
… their RTX 5090 mutation runs Each of 0ac4349, 433d88e and 598d835 now states what its guard does on a card at 700c03a with the fix in place and with it removed. The overflow row also says what the old kernel's dropped cell costs against the CPU reference (2 of 40 cases outside tolerance), and which caller can reach it.
… range that names no tile c56099b's message printed first and first + n - 1. An expert that takes no tile still trips the guard once the running offset is already past the bound, and then it printed "takes column tiles 64 to 63", which names nothing. Found by the guard's own R3 run on an RTX 5090: with ntiles_max halved at the launch, the trap fired with exactly that line. It now prints the count and where it starts. Diagnostic only; the condition and the trap are unchanged.
…they reach, measured not assumed The comment implied they exercise the ring's own quantize of y. They do not. With the ring's q8_1 block scale doubled on an RTX 5090, the Q8_0 chain cases at 2 and 3 tokens fail at ERR 0.25 against a tolerance of 0.005, and the 8-token case passes: 64 vectors of 4,096 columns leave the shared-memory plan no room for y, so ggml_cuda_mmvq_moe_keeps_y is false and y comes from the shared path. The cases still sit where the comment's first sentence says, at a full pair table, and still compare against the CPU reference. Only the claim about which path they cover was wrong.
…mutation and reachability runs The tile-list row claimed its R3 before the run; this is the run. All three are at 2026-10-06 on an RTX 5090 with rig's flags: - the ring's q8_1 block scale doubled fails the Q8_0 chain cases at 2 and 3 tokens at ERR 0.25 against a tolerance of 0.005, with 6 of 17 IQ3_XXS cases, and the restored build passes all 14; - the tile list handed half the launch's column tiles traps on n_mats=64,n_used=8 at exit 134, and that run is what corrected the message to name a count rather than an empty range; - the top-k aliasing rule, which no pass or fail can reach for, was instrumented: 26 hits and only ever at 4 rows, 0 under GGML_CUDA_TOPK_MOE_ALIAS_LEGACY=1.
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
1 participant
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.
A full review of this fork's changes against the base. Every fix carries a test that fails without it, or says plainly that it does not and why.
Server
3472162a7bad UTF-8 inside a tool call's string argument still answered 500.b915c5e6f'sutf8_replace_malformedjudged a sequence by its bit pattern, so a surrogate (ED A0 80), an overlong form (C0 80,E0 80 80,F0 80 80 80), a code point past U+10FFFF (F4 90 80 80) and a lead that leads nothing (F5-FF) all passed as well formed. The chat parser's strict JSON dump then threw inside a<parameter=...>value, which is the failure that commit was written to remove. The second byte is now checked against its range (Unicode Table 3-7), a malformed sequence is one U+FFFD for its maximal subpart, and a tail is held only while every byte of it is right so far.test-utf8-replace, new: 18 cases, 10 of which fail on the old function.cdd2dff19,ddaaf00a7a slot could draft from the conversation before it.4ec3d003bgave the MTP head's kept h row the position it is for, and a position is not an identity. The drafter implemented neitherget_statenorset_state, so a context checkpoint's stash was always empty for it. The row and its position now travel with the state they belong to:server_prompt_datacarries that blob beside the target's and the draft's, andprompt_clearclears it.ddaaf00a7then clears a slot's drafter pointer when the drafter is destroyed, which the first commit left dangling, since the slots outlive it and sleeping destroys it. The cost was draft acceptance, never the text.K-pool and sparse attention
00e9132dda sequence's pooled keys are marked stale per sequence, and a ubatch clears only its own.47fb5e66dthe emitted bitmap is made only on the direct path.caaad776ea rebuild compresses the pooled keys in chunks of 4,096 pools.0ac4349ean_kv_maxis sized on the longest tail the cells actually hold, not onr - 1. A sequence holding two cells at one position has a longer tail, and the compaction kept the firstn_kv_maxcells of a row with no signal. Both compaction kernels now trap instead. This is a wrong answer, not just a missing guard: with the trap removed and everyn_kv_maxcase given one cell too many, 2 of 40 exceed tolerance atn_kv_max512 (ERR 0.00207 and 0.00235 against 0.0005), and the 38 that pass do so only because one dropped cell sits under the bound at the larger values. Reachable by allama_decodecaller, sincellama_batch_allocrkeeps positions in a set and refuses only a decrease (llama-batch.cpp:316,:381); whether a server request reaches it was not measured.60ee43721GGML_CUDA_FATTN_SPARSE_LEGACY=1restores the dense kernel, which still reads live tiles. The whole-cache baseline needsGGML_CUDA_FATTN_LIVE_TILES_LEGACY=1as well, and both places now say so.MoE and matmul kernels
466d8d4f1the ring's consumers quantize to a warp-whole bound. Their shuffles name all 32 lanes, so a bound that split a warp would hang it. No graph reaches that today, and the kernel no longer leans on that.458b5c1c2,c7f958027the fused top-k's outputs may overlap its logits alone. The block barrier orders only the logits' reads, and the selection bias is read past it while a sibling warp writes.c7f958027then narrowed the test from containment to identity: the kernel readslogits->ne[1]rows while the rule is handedggml_nrows, so the unread tail of a logits tensor withne[2] > 1passed containment, as a bias that was ever a view of the logits would have. Neither is reachable here. Also: a PQ2_0 group takes computed, non-empty nodes only; the empty-graph key no longer readsnodes[0]; andGGML_CUDA_TIME_LAUNCH,GGML_CUDA_COUNT_INFLIGHTand the meta-capture test'sGGML_META_CAPTURE_LEGACYall parse asggml_env_switchdoes, where=trueor=onleft them silently off.c56099b8d,793ed1e75aMUL_MAT_IDtile list past the launch's column tiles traps. The bound's first term counts an expert at most once for a token, which every graph here satisfies and nothing enforced: four experts holding 5, 5, 5 and 1 of 16 columns at J = 4 need 7 tiles where the term allows 4, and the kernel wrote what fit and dropped the rest.793ed1e75fixes the message the guard's own mutation run exposed, which named an empty range.433d88eaethe hyper-connection front's mix RMS clamps its Gram quadratic form at 0, where a negative sum underrsqrtfgave NaN.598d83508the chunked KDA path states its log-decay precondition and enforces it.Tests
test-backend-opsgains the routed FFN chain in Q8_0 at 2 and 3 tokens and at 8 tokens on 8 experts, aTOPK_MOEshape at 4 rows, aDSV4_HC_PRE_FUSEDcase whose streams cancel, and four KDA cases. Two of them exist because the thing they test could not fail before: no registered top-k shape reached the 2-to-4-row aliasing rule, every one having 1 row or 22, and noDSV4_HCcase reached a negative Gram form, so the clamp's mutation would have passed vacuously.Verification
RTX 5090 on vast, built with rig's flags. At
ce77b5fd8, the tip:MOE_FFN_CHAIN14/14 andMUL_MAT_ID1096/1096. At700c03ac4:TOPK_MOE352/352,MUL_MAT1475/1475,MUL_MAT_VEC_FUSION1032/1032,MOE_FFN_CHAIN14/14, and on a second box the sameTOPK_MOE352/352 withFLASH_ATTN_EXT3260/3260,GATED_DELTA_NET83/83 and the sixDSV4_HCops. Atddaaf00a7:test-utf8-replaceok, the chain underGGML_CUDA_MMVQ_MOE_QUANTIZE_CHECK14/14, andtest-backend-meta-captureunderGGML_META_CAPTURE_LEGACY=0reporting 12 of 12 executables, which read FAIL before. CPU:test-kpool-input(733 builds with a tail overr - 1, each within the new bound) andtest-kpool-can-reuseok, and the server suites (prompt cache, slots, context shift, speculative, basic) pass, 54 in all.Each guard was then shown able to fail, on the same hardware:
n_kv_max + 1finite cellsmmvq_moe_quantize_yMUL_MAT_IDtile listntiles_maxhalved at the launchOne claim could not be settled by pass or fail, because refusing a fusion is always correct: whether any test reaches the top-k aliasing rule. It was measured instead, with the rule printing the row count it runs at. 26 hits, every one at 4 rows, and 0 under
GGML_CUDA_TOPK_MOE_ALIAS_LEGACY=1.