fix/feat(ds4): ROCmFPX byte-safe decode + fused-decode path + ds4 optimizations

[davide221]

Summary

Server-side DeepSeek V4 Flash changes (the byte-safe subset of #494), rebased onto the vendored ROCmFPX ggml tree with no submodule pointer. The six touched server files are byte-identical to the validated #494 tip.

Stacked on #502. Base is codex/rocmfpx-ggml-vendor so this PR shows only the server diff. The server code depends on symbols added there (ggml_ds4_hc_pre/post/out, ggml_swiglu_ds4_split), so #502 must merge first (or they merge together). Retarget to main once #502 lands.

Correctness (default ON)

• Token-by-token prefill by default — chunked prefill is only exact inside the raw SWA ring, so long prompts / multi-turn degraded. Chunked stays available via DFLASH_DS4_CHUNKED_PREFILL for short-prompt benchmarking.
• Clear the cache buffer at new-sequence prefill (kv_offset==0) so requests 2..N are byte-stable instead of pooling over leftover compressor state.
• Route non-hybrid (all-hot) placement through the HC-complete layer-range path; deepseek4_step's non-hybrid branch is HC-less and produces garbage on all-hot boxes.
• Key the cached decode-attn graph on the flush pattern; the old key collided once the compressed-KV ring filled and reused a stale-topology graph.
• Read compressor state / comp-cache through the ggml_set_rows result tensors so current-step writes are explicit graph dependencies.
• Default DeepSeek4 chat prefix when the request has no system message (explicit caller system prompts preserved).

Perf — byte-identical default

• Persistent HC matvec pool (row-split keeps per-row accumulation order) + f16c dot kernels with scalar-order adds.
• Per-step decode scalar inputs uploaded in 2 tensor_sets instead of ~430.

Opt-in, default OFF (documented as not bit-identical)

• DFLASH_DS4_FUSED_DECODE single-graph decode with GGML_OP_DS4_HC.
• DFLASH_DS4_FFN_RAW_MMID / DFLASH_DS4_FFN_FUSED_COMBINE accumulation reordering.
• DFLASH_DS4_ROCMFPX_HC_GPU GPU HC pre-mix.

Note

The default HC matvec pool spins-then-yields between tokens (idle CPU burn when the server is quiet); a condvar park is a reasonable follow-up but is left as-is here to keep the validated numbers.

[cubic-dev-ai]

2 issues found across 6 files

Not reviewed (too large): server/src/deepseek4/deepseek4_graph.cpp (~1,745 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="server/src/deepseek4/deepseek4_internal.h">

<violation number="1" location="server/src/deepseek4/deepseek4_internal.h:67">
P3: Fused/full-graph timing is collected per step but disappears from the aggregate timing output because the newly added `full_graph_*` fields are not included in `add_step_tel(...)` or `log_step_tel(...)`. If `DFLASH_DS4_TIMING` is used to evaluate the fused decode path, these counters will remain invisible at the phase level; consider aggregating and logging them with the rest of `DeepSeek4StepTelemetry`.</violation>
</file>

<file name="server/src/deepseek4/deepseek4_backend.cpp">

<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:556">
P1: New local-server requests can still see a stale compressed-cache topology after the buffer clear: `ggml_backend_buffer_clear(cache_.buf, 0)` zeros the tensor memory but leaves `cache_.layers[*].n_comp` and `n_index_comp` from the previous request. Since those counters control how many compressed rows attention reads, request 2+ can attend over zeroed-but-counted compressed rows instead of matching a fresh cache. It would be safer to reset `cache_.cur_pos`, `n_comp`, and `n_index_comp` alongside the buffer clear, matching the layer-split reset paths.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: New local-server requests can still see a stale compressed-cache topology after the buffer clear: ggml_backend_buffer_clear(cache_.buf, 0) zeros the tensor memory but leaves cache_.layers[*].n_comp and n_index_comp from the previous request. Since those counters control how many compressed rows attention reads, request 2+ can attend over zeroed-but-counted compressed rows instead of matching a fresh cache. It would be safer to reset cache_.cur_pos, n_comp, and n_index_comp alongside the buffer clear, matching the layer-split reset paths.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_backend.cpp, line 556:

<comment>New local-server requests can still see a stale compressed-cache topology after the buffer clear: `ggml_backend_buffer_clear(cache_.buf, 0)` zeros the tensor memory but leaves `cache_.layers[*].n_comp` and `n_index_comp` from the previous request. Since those counters control how many compressed rows attention reads, request 2+ can attend over zeroed-but-counted compressed rows instead of matching a fresh cache. It would be safer to reset `cache_.cur_pos`, `n_comp`, and `n_index_comp` alongside the buffer clear, matching the layer-split reset paths.</comment>

<file context>
@@ -535,9 +535,26 @@ int DeepSeek4Backend::do_prefill(const std::vector<int32_t> & tokens,
+    // previous request's leftover state rows and outputs from the 2nd/3rd
+    // request on can drift by a token or two.
+    if (kv_offset == 0 && cache_.buf) {
+        ggml_backend_buffer_clear(cache_.buf, 0);
+    }
     last_logits_.clear();
</file context>

Suggested change

	ggml_backend_buffer_clear(cache_.buf, 0);
	ggml_backend_buffer_clear(cache_.buf, 0);
	cache_.cur_pos = 0;
	for (auto & lc : cache_.layers) {
	lc.n_comp = 0;
	lc.n_index_comp = 0;
	}

[cubic-dev-ai]

P3: Fused/full-graph timing is collected per step but disappears from the aggregate timing output because the newly added full_graph_* fields are not included in add_step_tel(...) or log_step_tel(...). If DFLASH_DS4_TIMING is used to evaluate the fused decode path, these counters will remain invisible at the phase level; consider aggregating and logging them with the rest of DeepSeek4StepTelemetry.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_internal.h, line 67:

<comment>Fused/full-graph timing is collected per step but disappears from the aggregate timing output because the newly added `full_graph_*` fields are not included in `add_step_tel(...)` or `log_step_tel(...)`. If `DFLASH_DS4_TIMING` is used to evaluate the fused decode path, these counters will remain invisible at the phase level; consider aggregating and logging them with the rest of `DeepSeek4StepTelemetry`.</comment>

<file context>
@@ -64,6 +64,11 @@ struct DeepSeek4StepTelemetry {
     uint64_t output_us = 0;
     uint64_t sample_us = 0;
     uint64_t emit_us = 0;
+    uint64_t full_graph_build_us = 0;
+    uint64_t full_graph_alloc_us = 0;
+    uint64_t full_graph_set_us = 0;
</file context>

[davide221]

Validation update (2026-07-09): validated on lucebox2 against DeepSeek-V4-Flash ROCmFP2. The env-only expert routing knob DFLASH_DS4_TOPK=4 reduces routed experts from 6 to 4 and renormalizes retained weights. AR validation (GSM n=5 plus Math n=5) reached 26.19 tok/s GSM and 25.16 tok/s Math, both 5/5, versus baseline 23.54 and 22.24 tok/s. DSpark fused speculative decode reached 20.1–20.3 tok/s warm in repeated q=4 runs, with approximately 105–106 ms verify steps. Adaptive width, causal batched verify, fused verify, and interior-boundary compressor emission are wired and producing coherent output. The neuron-magnitude mask preserved the small-screen scores but produced no speedup; the block-sparse follow-up was not quality-safe and is kept out of the validated path. These measurements come from the hub-494 experiment worktree; the final k=4 patch and larger DSpark runtime follow-on still require explicit commit/push if they are to become part of the PR diff.

[cubic-dev-ai]

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="server/src/deepseek4/deepseek4_graph.cpp">

<violation number="1">
P2: `DFLASH_DS4_TOPK` will not reduce expert work for hash-routed DS4 layers: this override only changes `build_moe_routing`, while the hash-routed decode graphs still allocate/upload `w.n_expert_used` expert IDs and run `ggml_mul_mat_id` over that full count. If the knob is intended to be a serving-wide bandwidth lever, it would be worth applying the same reduced count when building and filling `hash_ids`, or documenting that hash-routed layers are excluded.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[DeanoC]

Semi-Automated review pass.
Codex: Overall this is careful, well-documented work and the risky paths (fused decode, FFN combine variants, GPU HC) are correctly gated behind default-OFF env flags. My comments concentrate on a few changes that land on the default code path plus one likely-dead block. Inline notes below;

Added a few from Codex, all just nits really. Works well, i've been tested it over the last few days and is works well!
Nice work davide!

[DeanoC]

Default-path reduction changed: sum_rows → repeat_back. This fires whenever FFN_FUSED_COMBINE is off, i.e. the default, yet it isn't in the "byte-identical default" list. Two things to confirm before merge:

1. Backend support — GGML_OP_REPEAT_BACK (f32) must be implemented on the vendored ROCmFPX/HIP backend. The old cont(permute)+sum_rows path was known-good; if repeat_back isn't wired up here, default decode fails at compute time. Given the PR's premise is ROCmFPX byte-safety, this is the top risk.
2. Bit-identity — both sum the same n_used values per (n_embd, n_tokens) output, so it's mathematically equal, but kernel summation order/rounding can differ. If you validated logit/argmax equality vs the fix(ds4): keep ROCmFPX generation byte-safe #494 tip, please note it.

Same change also at the decode variant in build_cached_decode_ffn_graph (~line 3739).

[DeanoC]

Default system prompt is correctly emitted (via result += system_content below, regardless of has_system), but two small things:

• Missing separator with tools: when has_tools && !has_system, the if (has_system) result += "\n\n"; above is skipped, so tools_json is concatenated directly onto this default prompt. Setting has_system = true inside this block fixes the separator and is more honest (there now is system content).
• Behavior change: every DeepSeek4 request without a system message now silently gets this prompt. That's product-visible for API callers who deliberately send none. Please confirm it matches the ds4 reference server's default.

[DeanoC]

Possibly-dead code. do_prefill/do_decode now route the non-hybrid case to deepseek4_step_layer_range, and deepseek4_step early-returns to deepseek4_step_hybrid for the hybrid case. That leaves this full-step decode-graph reuse machinery (reuse_full_step_decode / full_step_cache) reachable only if deepseek4_step is still called with a non-hybrid model somewhere (tests/bench/tools). If it isn't, this is a fair amount of new, unexercised complexity on a path the PR description itself calls an HC-less stub. Please confirm the caller or drop it.

[DeanoC]

Nice — this is a genuine latent-bug fix, not just a refactor. The old single-token decode read the first n_raw-1 physical ring rows from offset 0, which was wrong once kv_start >= n_swa (included a stale row, dropped the newest). The tail+head split reads every row except the current slot; since key attention is permutation-invariant, membership is what matters, so this is correct. Worth a regression test that decodes past kv_start > n_swa.