perf(ds4): default mixed ROCmFP MMQ prefill on gfx1151 - #683
Conversation
|
Experimental long-context follow-up (still draft/opt-in): added a reusable D512 K==V streaming top-k HIP attention path behind DFLASH_DS4_DIRECT_INDEXER_TOPK=1 and GGML_CUDA_MLA_STREAM_TOPK=1. On gfx1151, three matched isolated graph runs improved 2627.3→2236.8 us (1.17x), 2645.5→2293.5 us (1.15x), and 2649.7→2363.3 us (1.12x). Matched ROCprof kernel time improved 2619.5→2126.6 us (-18.8%). test_deepseek4_unit and all 16 DS4 benchmark-tool tests pass. No full-model tok/s or output-parity claim yet; Lucebox8 is currently unreachable and the fallback host is booted with only ~61 GiB system RAM. |
|
Pushed Measured on gfx1151:
The MMQ selector is format-aware and only activates for sparse grouped MoE on gfx1151 via Validation: 76 grouped-MMID parity cases pass, masked routes remain +0, the ROCmFPX numerical suite passes, and |
|
Pushed b0d5b1b: vectorized FP32 staging for the reusable D512 streaming indexed-attention candidate. Qualification on gfx1151:
Enable with GGML_CUDA_MLA_STREAM_F32_STAGE=1 alongside the existing direct-indexer and streaming-attention flags. It remains opt-in because LB8 is still unreachable and the full 8K/32K/128K model-backed throughput/output A/B is pending. |
|
Pushed 1b6cd67: coalesced ROCmFP3 MMQ weight loads. For each eight FP3 values, one wave lane now reads the exact three packed bytes once and expands both adjacent int8 groups. On HIP, the three bytes come from one safe four-byte memcpy load within the 14-byte quant block. Matched DS4 sparse grouped shape (K=4096, rows=2048, 256 experts, top-k 6, width 48): q3 mean 6.268 ms -> 6.114 ms (-2.46%); final 20-iteration run 6.111 ms. Q2/Q4 controls were unchanged. Grouped-output parity passed all 76 cases, the complete ROCmFP numerical suite passed, and dflash_server builds. I also tested folding scale staging into the weight pass; it regressed Q2/Q3 by more than 2x and was discarded. |
|
Pushed f140639: paired ROCmFP2 MMQ loads. Adjacent FP2 four-value groups now share one aligned 16-bit HIP load per lane. On the matched sparse grouped shape, two 20-iteration runs averaged 4.894 ms before and 4.835 ms after (-1.20%). Q3/Q4 controls were unchanged. All 76 grouped cases passed, the complete ROCmFP numerical suite passed, and dflash_server builds. |
|
Pushed Three alternating matched runs at the DS4 prefill shape (K=4096, rows=2048, 256 experts, top-k 6, width 3072) averaged:
All baseline/candidate outputs across three runs had the same SHA-256. Additional outputs were byte-identical at widths 128/256/512/1024; all 76 grouped-MMID cases and the complete ROCmFP numerical suite pass, and |
|
Pushed |
|
First review fix: The previous loop stopped after the first eligible tile because selecting it changed the loop's continuation condition. Default dispatch and rejected overrides now examine wider candidates; valid explicit/adaptive recommendations still return immediately. Hardware/shared-memory eligibility, automatic-only exclusions, and smallest-width tie breaking are preserved. Added six regression cases using the same host selector as production dispatch, registered them in Validation on lucebox8, Radeon 8060S / gfx1151, ROCm 7.2.4, native ROCmFP wire-format build:
This is a focused first fix, not merge approval. The Windows HIP environment helper, process-global approximate-mode policy leak, and reconciliation with current main remain outstanding. No end-to-end throughput or new Vulkan comparison was measured in this patch. Reproducible test build and logs remain in |
|
Pushed This addresses the three outstanding blockers from the previous review:
Validation on lucebox8, Radeon 8060S / gfx1151, ROCm 7.2.4:
The optimization kernels are retained. Fresh matched client throughput checks are in progress; this comment is not a throughput claim or merge approval. A numeric-tokenization fix on main changed the old counting benchmark's input tokens, so the new comparison uses a number-free prose task and asserts identical raw input token IDs between the old/new tokenizers. Six experts, sparse prefill, cold caches; AR and adaptive DSpark are checked separately. Reproduction source/build/logs: @cubic-dev-ai please review the model-local policy, fallback/capture consistency and main reconciliation at |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 27 files
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/docs/DS4.md">
<violation number="1" location="server/docs/DS4.md:609">
P3: The new DS4.md section documents DFLASH_DS4_DIRECT_INDEXER_TOPK and GGML_CUDA_MLA_STREAM_TOPK/GGML_DS4_FA_STREAM_TOPK, but these are absent from the full control inventory in server/docs/ENVIRONMENT.md while the sibling streaming variables (GGML_CUDA_MLA_STREAM_F32_STAGE/FAST_EXP) are listed there. Add each newly surfaced control to the ENVIRONMENT.md inventory with its defining file so the registry stays the single source of truth.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_backend.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:924">
P1: On gfx1151 approximate prefill, this model-wide policy automatically admits qtype-106 MMQ for down projections, although the qualified default is only the matched routed gate/up fusion. Restrict automatic qtype-106 admission to the gate/up path with its ids and gfx1151 checks, keep down projections on their generic/default predicate, and preserve the explicit override including `=0`.
(Based on your team's feedback about qtype-106 MMQ admission.)</violation>
</file>
<file name="server/deps/llama.cpp/ggml/src/ggml-cuda/fattn.cu">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml-cuda/fattn.cu:2665">
P3: This change adds new backend control surfaces GGML_CUDA_MLA_STREAM_TOPK, GGML_DS4_FA_STREAM_TOPK, GGML_CUDA_MLA_STREAM_F32_STAGE, GGML_CUDA_MLA_STREAM_FAST_EXP (and DFLASH_MOE_COMBINE_VEC4 in moe-fused.cu). Per the repo convention, new GGML/DFLASH env names should be added to the generated environment inventory in server/docs/ENVIRONMENT.md with their defining file and regeneration guidance. Confirm the docs portion of this PR inventories all of them; if not, add them.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| w_.routed_expert_top_k = cfg_.expert_top_k; | ||
| if (!moe_hybrid_) { | ||
| w_.mixed_mmq_policy = gfx1151_mix_mmq_prefill_policy( |
There was a problem hiding this comment.
P1: On gfx1151 approximate prefill, this model-wide policy automatically admits qtype-106 MMQ for down projections, although the qualified default is only the matched routed gate/up fusion. Restrict automatic qtype-106 admission to the gate/up path with its ids and gfx1151 checks, keep down projections on their generic/default predicate, and preserve the explicit override including =0.
(Based on your team's feedback about qtype-106 MMQ admission.)
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 924:
<comment>On gfx1151 approximate prefill, this model-wide policy automatically admits qtype-106 MMQ for down projections, although the qualified default is only the matched routed gate/up fusion. Restrict automatic qtype-106 admission to the gate/up path with its ids and gfx1151 checks, keep down projections on their generic/default predicate, and preserve the explicit override including `=0`.
(Based on your team's feedback about qtype-106 MMQ admission.) </comment>
<file context>
@@ -883,6 +920,14 @@ bool DeepSeek4Backend::load_model() {
}
w_.routed_expert_top_k = cfg_.expert_top_k;
+ if (!moe_hybrid_) {
+ w_.mixed_mmq_policy = gfx1151_mix_mmq_prefill_policy(
+ cfg_.device.gpu, cfg_.prefill_mode);
+ }
</file context>
There was a problem hiding this comment.
This appears to conflate the existing default in #667 with this PR’s explicitly approximate prefill mode. #683 already enabled registry-aware MMQ for both mixed qtypes and all routed projections through its gfx1151 dense/sparse automatic environment setting (cd8abb8); this fix replaces that process-global state with the same model-local decision. Monolithic Exact and other devices still resolve to DEFAULT, preserving #667’s restricted paired path; explicit =0 resolves to DISABLED. Restricting the approximate path to gate/up here would undo part of #683 rather than preserve its behavior. The new FP2/FP3 dispatch and fallback tests pass, and the matched six-expert 8K model checks preserve throughput. Broader quality qualification remains a draft gate. Please distinguish any reproducible admission outside that approximate mode from the intended #683 policy.
|
Pushed
Fresh lucebox8 gfx1151 qualification: 494/494 CTest entries passed, comprising all 489 server-unit entries and the affected DS4/grouped-MMID/MMQ/fused-combine checks plus the parent-width rejection test. Release HIP server and test builds pass. The new streaming launch assertions passed, not merely the numerical fallback checks. The proposed P1 restriction is answered in-thread: #667's restricted default and #683's explicitly approximate-prefill policy are different scopes. This patch retains #683's prior approximate-mode admission and gains; it does not enable that general mixed-MMQ path for monolithic Exact mode. Please re-evaluate against that original scope rather than narrowing the approximate path by assumption. The six-expert, exact-input-token 8K checks on the preceding reconciliation head preserved both AR and fused/adaptive throughput. The final cached-policy follow-up is being rerun before recording the final numbers. The unoptimized/unfused old-head diagnostic also exposed an existing second-request crash; those results are kept separate and are not a matched optimized comparison. @cubic-dev-ai please re-review |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
|
Final before/after check is complete on lucebox8, Strix Halo gfx1151, using pushed head Baseline: previous PR head
Conditions:
Final correctness/build checks: 494/494 CTest entries passed on gfx1151, including actual streaming-launch assertions, interleaved model-policy isolation, cached hot/cold graph policy, mixed FP2/FP3 numerical/dispatch tests, grouped MMID and fused combine. Independent Limitations: this small 8K sample shows no throughput regression, but 128K and Vulkan were not remeasured. Approximate-mode outputs differ between builds, so this is not a byte-identity or broad quality claim. Adaptive remains slower than AR on this prose task. The separate old-head unfused-verification diagnostic crash reported earlier is excluded from this matched fused comparison, not silently treated as a passing test. Artifacts remain on LB8 at The historical longer-context gains remain separately labeled in the PR description. Still draft: fresh CI/Cubic, broader quality, long-context and stability qualification remain open. The qtype-106 P1 scope question is answered with evidence in its thread, not counted as an approval. |
There was a problem hiding this comment.
3 issues found across 29 files
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/docs/ENVIRONMENT.md">
<violation number="1" location="server/docs/ENVIRONMENT.md:222">
P3: `DFLASH_MOE_COMBINE_VEC4` appears twice in the generated full inventory: once here (line 222, with a plain-text description instead of the defining file) and again immediately after `DFLASH_MOE_FUSED_COMBINE` (line 240, `- ggml-cuda/moe-fused.cu`). The grep-based `getenv` inventory format is `- VAR - file`, and `DFLASH_MOE_COMBINE_VEC4` is only defined in `ggml-cuda/moe-fused.cu`. Remove this line and keep the single correct `- ggml-cuda/moe-fused.cu` entry.</violation>
</file>
<file name="server/src/common/moe_hybrid_ffn_eval.cpp">
<violation number="1" location="server/src/common/moe_hybrid_ffn_eval.cpp:24">
P2: set_graph_mixed_mmq_policy stamps the mixed-MMQ policy on every MUL_MAT/MUL_MAT_ID/MUL_MAT_GROUPED_SRC in the whole graph, including non-routed nodes such as the shared expert's plain ggml_mul_mat. Those are not mixed-expert matmuls, so the ENABLED policy is applied outside the routed gate/up/down scope that the per-op ggml_mul_mat_set_mixed_mmq calls target elsewhere. Restrict the traversal to routed ops (or rely on the per-op calls) to avoid over-applying the policy.</violation>
</file>
<file name="server/test/test_deepseek4_mmid_grouped_cuda.cpp">
<violation number="1" location="server/test/test_deepseek4_mmid_grouped_cuda.cpp:166">
P3: When a graph compute fails mid-benchmark, the printed average divides the elapsed time by the full requested iteration count even though fewer iterations ran, understating the reported per-iteration cost. Track the count of successfully executed iterations and divide by that instead. This affects the benchmark printout in both run_case() and run_combine_graph().</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Pushed Review fixes
VerificationFinal #704-integrated head on lucebox8/gfx1151, ROCm 7.2.4, HIP graphs ON: Release server/test build passes and 495/495 CTest entries pass (37.14 seconds), including all 489 server-unit entries and the affected DS4, mixed-MMQ, grouped-MMID, fused-combine and negative/self-test checks. No test tolerance was weakened. Pre-integration checks also passed. The policy-scope fail-before receipt and all build/test logs are retained under No GPU kernel source changed in this follow-up. Before integrating #704, the HIP shared library was byte-identical to the measured For clarity, the September 9 matched retrieval recheck of the preceding
Six experts, sparse F16 fused verification, identical full input token IDs, all eight measured responses strict 5/5 and the same expected response hash. Adaptive was enabled but stayed q4 on this high-acceptance retrieval task. This does not resolve the slower variable-acceptance prose case or qualify 128K/Vulkan parity. Complete provenance and limitations are in The qtype-106 scope discussion remains explicit: #683 intentionally qualifies approximate gfx1151 routed prefill, including down projections; #667's narrower Exact/default paired admission remains unchanged. This follow-up fixes the separate shared-expert metadata issue without silently undoing #683's intended approximate policy. Please review that distinction against the existing mode-qualification and FP2/FP3 dispatch/numerical tests. @cubic-dev-ai please re-review the current head. The preceding head passed Linux, Windows, Strix, R9700, DGX and workspace CI; RTX3090 was cancelled, not passed. Fresh CI/review on this new head is still required. Still draft; no merge or auto-merge enabled. |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 29 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Fixed both latest review findings in
Verification on lucebox8 / gfx1151 / ROCm 7.2.4:
Build/fail-before/final receipts are retained in @cubic-dev-ai please re-review the current head, especially these two findings. The earlier approximate-MMQ policy-scope discussion and fresh cross-platform CI remain separate merge gates. Still draft; no merge or auto-merge enabled. |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 30 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
Fixed the three latest test-harness findings in
Lucebox8 / gfx1151 / ROCm 7.2.4 validation:
Receipts are retained on LB8 under Fresh CI and automatic Cubic review apply to this new commit. No additional manual re-review request is being sent. The older approximate-MMQ default-policy discussion remains separate and open. Still draft; no merge or auto-merge enabled. |
1 participant
Summary
Builds on the now-merged #667 and targets
main. This makes its registry-aware mixed ROCmFP MMQ prefill path the default on gfx1151 when DS4 uses dense or sparse approximate prefill.The old path dequantized mixed expert weights and then called BLAS. The direct MMQ path removes that round trip and reduces the 8K trace from 325,841 to 73,165 GPU dispatches.
Exact prefill and non-gfx1151 devices keep their existing behavior. An explicit DFLASH_DS4_MIX_MMQ_PREFILL=0 remains the kill switch.
Historical Strix Halo results
DeepSeek-V4-Flash-0731 ROCmFPX MIX, six routed experts, chunk 3072, sparse prefill, DSpark enabled, cold cache:
The historical 8K result was a matched same-code kill-switch A/B. The longer comparisons used the same model, service flags, tokenizer-built prompt, and HTTP workload from the pre-change #667 captures. All measured contexts in that sweep produced the same response SHA-256: 16fbd7d1f18d2fedb247d73edc3bc6aa040f5ab99bd3b48c35b79e543d22179b. These are not fresh results for the current head.
The gain narrows with context length because sparse attention becomes the remaining dominant cost. This does not claim Vulkan parity yet.
Reuse and safety
Historical verification
September 8 fix verification
The fixes through
ed09c8682make automatic mixed-MMQ selection model/graph-local, remove the Windows HIP environment mutation, reconcile this PR with current main, and propagate the policy through cached/hash-routed and single-token fallback graphs. The streaming-attention tests now verify actual kernel launches. The optimized kernels and explicit disable switch are retained.Fresh lucebox8 gfx1151 validation: 494/494 CTest entries passed (489 server-unit entries plus affected DS4/MMQ/grouped-MMID/fused-combine and negative-option tests). HIP server and test builds passed.
Matched 8K prose-task regression check, old PR head
710315f44versus final headed09c8682: two cold requests per build/mode, identical input token IDs, six routed experts, 256 output tokens, sparse approximate prefill, chunk 3072, caches disabled. Throughput is total tokens divided by total API-reported stage time.Adaptive runs use confidence-driven width (cap 4), fused verification and F16 KV in both builds. These results show no slowdown in this small regression sample; they do not isolate the policy fix from other changes brought in from main. Outputs are not byte-identical between builds, and adaptive decode remains slower than AR on this task. No fresh 128K or Vulkan comparison was performed.
Still draft pending fresh CI/Cubic review, broader output-quality and long-duration stability qualification.