DeepSeek-V4 FP4: fused_compress FP4 scatter + rmsnorm_rope_rotate FP4 KV-cache kernel#4029
DeepSeek-V4 FP4: fused_compress FP4 scatter + rmsnorm_rope_rotate FP4 KV-cache kernel#4029junhaha666 wants to merge 16 commits into
Conversation
…ilders
After the FP4 merge the internal builders carried three overlapping quant
flags (quant bool + quant_mode str + quant_fp4 bool), which was redundant and
error-prone. Collapse them: _build_kernel / _build_kernel_ksplit and both
compile_* functions now take a single quant_mode ("none" | "per_row_fp8" |
"group_fp8" | "fp4") and derive quant / quant_fp4 / nm_asm internally.
The public flydsl_fused_compress_attn keeps the legacy `quant` bool for
backward compatibility (ATOM and the op-test still pass it); it resolves the
master quant_mode and forwards it straight to the builders, dropping the
intermediate _builder_quant_mode mapping and the per-call quant=/quant_fp4=
arguments.
No behavior change: bf16 / per-row fp8 / group_fp8 (nm-asm) / fp4 scatter all
verified bit-exact against the torch reference (csa_main, csa_indexer,
csa_indexer_fp4, nm_asm_fp8).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…reader kv_scale_preshuffle_offset wrote the e8m0 scale linearly (+ pos_in_block), but the pa_mqa_logits_fp4 reader loads the scale with a packed-dword access that expects the NTPW N-tiles of a token to be contiguous, i.e. an interleaved layout sflat = (pos%16) * (kv_block_size/16) + pos/16. With the linear layout every scale was read from the wrong byte -> wrong dequant in the FP4 indexer. Use the interleaved sflat (multiplier = tiles-per-block = kv_block_size/16, which equals the reader's NTPW whenever N_PHYS==1). It degenerates to the old linear layout for kv_block_size==16 and is (pos%16)*4 + pos/16 for the FP4 indexer's k1_csa==64. The data axis is unchanged (already linear). Update the op-test reference to the same interleaved layout. Verified: rmsnorm_rope_rotate_activation_fp4quant_kvcache vs torch ref now passes for kv_block_size 16 and 64, do_rotate_act False/True. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norm_weight is already counted via tensor_nbytes() in the bandwidth calc, so weight_bytes was dead. Drop it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
…ache_new # Conflicts: # aiter/ops/flydsl/kernels/fused_compress_attn.py
Allow skipping the Hadamard rotate in the fused RoPE(+quant) path. The
rotate lives in the HIP kernels, so plumb a `do_rotate_act` (default true,
preserving current behavior) through the full stack:
- dsv4_rotate_quant.cu: add `bool do_rotate_act` template param to both
rope_hadamard_rotate_activation_{fp4,fp8}quant_kernel, guarding the
Hadamard block with `if constexpr`; split dispatch macros into a
compile-time `_IMPL_` plus a runtime `if(do_rotate_act){...}else{...}`
`_IMPL`; add the param to the 3 launchers.
- dsv4_rotate_quant.h / rocm_ops.hpp: add the param + pybind arg.
- quant.py: add `do_rotate_act: bool = True` to rope_rotate_activation and
the 3 @compile_ops stubs, forwarding to the underlying ops.
Validated on GPU (fp4/fp8 bit-exact, bf16 within rounding) for both modes:
do_rotate_act=False produces RoPE-only output matching the no-rotate ref.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi @junhaha666 — reviewed the FP4 changes in depth. Overall the quality is solid: 1. fp4 auto-K-split: the docstring contradicts the code — and I want to confirm the auto path is tested.
So in the default case (no explicit My concern isn't the contradiction itself, it's coverage: does the test suite exercise this auto path? The fp4 scatter/amax/e8m0 code in Could you (a) confirm the fp4 auto-K-split path is covered by a test that does NOT pass an explicit 2. KV e8m0 scale interleave layout — reader side isn't in this diff. The scale writer moves to the interleaved layout ( 3. Nothing else is blocking — arch handling, dispatch completeness, and boundary guards all look correct. Once the fp4 K-split coverage is confirmed, I'm happy to approve. |
flydsl_fused_compress_attn's docstring claimed the FP4 scatter path was "Legacy single-wave kernel only (never auto K-split)" and listed only BF16 and FP8 as auto-K-split shapes. This contradicts the code: the FP4 indexer shape (_is_csa_indexer_fp4) is in the auto-pick set, so with real block_tables and a small plan_capacity it auto-engages K-split (NW=4/2 via csa_ksplit_num_waves), same as the FP8 indexer. Update the docstring to match: FP4 is K-split-capable and auto-engages on the CSA Indexer geometry, reusing the FP8 wave-count heuristic (the win comes from parallelizing the dtype-agnostic online-softmax pool). Now consistent with the module docstring, the _is_csa_indexer_fp4 comment, and the op-test's csa_indexer_fp4 decode sweep (validated vs the pure-torch reference across the bs range, covering both the NW=4 K-split path and the NW=1 legacy fallback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/inferred finding discipline From a full day of real reviews (aiter#3802/#4098/#2565/#4029/#3593/#4171): - grep .cu+.cuh+.h, not just diff files, before claiming missing sync/branch - classify CI failures as infra-flake vs real regression before blaming the PR - tag findings [verified]/[inferred]; never ship an unconfirmed root cause
The K-split FP4 scatter (amax/e8m0/pack) in _build_kernel_ksplit mirrors the
single-wave _build_kernel emitter but runs on a different wave layout (K-split
lid 0-63, different group-reduce span). The shape sweep exercised it only
implicitly (auto-pick engages K-split at decode small-bs), so a reader could
not tell the auto K-split FP4 path was covered.
Add test_flydsl_csa_indexer_fp4_ksplit: drives flydsl_fused_compress_attn on
csa_indexer_fp4 WITHOUT an explicit k_split_num_waves and
1. asserts the auto-pick engages the multi-wave kernel (NW>1) for this
plan_capacity — so the run is guaranteed to hit K-split, not single-wave;
2. validates the auto (K-split) FP4 cache vs the pure-torch reference;
3. validates the forced-legacy (NW=1) FP4 cache vs the same reference, so the
two wave layouts are cross-checked against a common oracle.
Wired into main() as Table 4 (--fp4-ksplit-bs, default [2, 8]). Factor the
FP4 dequant+e8m0 compare out of test_flydsl_compress_attn into a shared
_check_fp4_cache helper (used by both the sweep and the new test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ fp4)
The K-split scatter (amax/e8m0/pack) in _build_kernel_ksplit mirrors the
single-wave _build_kernel emitter but runs on a different wave layout (K-split
lid 0-63, different group-reduce span). The shape sweep exercised the quantized
indexer K-split paths only implicitly (auto-pick engages it at decode small-bs),
so a reader could not tell they were covered.
Add test_flydsl_csa_indexer_ksplit(shape_label): drives
flydsl_fused_compress_attn on both the fp8 (csa_indexer) and fp4
(csa_indexer_fp4) indexer shapes WITHOUT an explicit k_split_num_waves and
1. asserts the auto-pick engages the multi-wave kernel (NW>1) for this
plan_capacity — so the run is guaranteed to hit K-split, not single-wave;
2. validates the auto (K-split) cache vs the pure-torch reference;
3. validates the forced-legacy (NW=1) cache vs the same reference, so the two
wave layouts are cross-checked against a common oracle.
Wired into main() as Table 4 (--ksplit-bs, default [2, 8], both shapes). Factor
the FP4 and FP8 cache compares out of test_flydsl_compress_attn into shared
_check_fp4_cache / _check_fp8_cache helpers (used by the sweep and the new test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…error, D10b FlyDSL (#4171) * review-pr skill v2: dispatch gap rule, arch-string FP fix, P5 timing error, D10b FlyDSL Key changes based on 191-PR test sweep (88 aiter + 93 ATOM): - B4 (new): "New dispatch value not handled by all paths, no warning" Highest-frequency finding (~18% of PRs). Covers: new dtype/arch/flag added to a multi-way dispatch where some branches silently fall through to wrong behavior with no assert/warning. Includes FP self-check to filter upstream guards. Old B4 (over-conservative assert) moved to B7. - C4 (new): "New GPU arch string literal in dispatch condition" FP self-check front-loaded: search unchanged lines first; if same arch string already exists → skip (pre-existing style). Fires only on genuinely new introductions. - D10b (new): "FlyDSL arith.bitcast requires arith.unwrap() on operand" Raw DSL value passed to arith.bitcast causes JIT type error at first invocation of that shape/dtype combo. Silent in Python, crashes at kernel JIT time. Real example: aiter#3944 bf16/f16 output path. - P5 (new): "Benchmark timing excludes one-time setup cost" Distinct from P1 (missing numbers). Covers: shuffle_weight, first-call JIT compile, precompile excluded from timing window → claimed speedup is actually a regression. Real examples: aiter#4166 (1.14x claim → 0.83x actual), aiter#3944 (7 precompile kernels on live stream). - Step 3 classification: wired B4/P5/D10b into trigger lines so new constexpr/routing-flag PRs and FlyDSL PRs auto-prompt the right checks. - Step 8 output format: findings now require three parts (Problem + Impact + Action verb). "Author must" / "Reviewer should ask" required; no verb = do not include in output. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * review-pr skill: B6→parametric family, G1b, D1b, C4 FlyDSL exemption - B6 restructured into a 6-row parametric table (param-discard, param-removed, repr-key, arch-discard, dispatch-silent, rename) — unified mental model for all API propagation incompleteness sub-types - G1b: blocking queue.get() without timeout in production serving code - D1b: Python-side UnboundLocalError from conditional variable assignment (ATOM#860 real example; static analysis blind spot) - C4: add FlyDSL capability guard exemption — get_gfx() inside a kernel-specific wrapper is not centralized dispatch hardcoding (aiter#3870) - Step 3 classification: new trigger entries for G1b and D1b Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * review-pr: add cross-file verification + CI-failure triage + verified/inferred finding discipline From a full day of real reviews (aiter#3802/#4098/#2565/#4029/#3593/#4171): - grep .cu+.cuh+.h, not just diff files, before claiming missing sync/branch - classify CI failures as infra-flake vs real regression before blaming the PR - tag findings [verified]/[inferred]; never ship an unconfirmed root cause * review-pr: add E4 (downstream CI coverage / ci:all) + E5 (stable-API owner sign-off) + Step-3 trigger E4: downstream tests (ATOM/SGLang/vLLM) are skip-by-default; a change to a downstream-consumed op can pass aiter CI and break downstream after merge (aiter#3459). Adds dispatch-reachability + model->label triage. E5: behavior/signature/dispatch changes to stable core APIs (fused_moe/mla/attention/quant/jit-core) need de-facto owner sign-off before merge, not a post-merge revert (aiter#3593). * review-pr: add global 🔴 gate — must name concrete triggering input before firing 🔴 Generalizes the per-rule FP self-check (present on B4/C4/B7) into a mandatory gate over all 🔴 findings, closing the gap on rules like D9 that omit one. A 🔴 with no demonstrable triggering shape/scale/input must be downgraded or dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: fix P5 — #4166 was a false positive, not a real example aiter#4166 preshuffles the static weight once outside the timing loop and honestly reports geomean 0.69x; it never claimed 1.14x. The old example taught the amortization error (charging a one-time cost per call). Reframe P5 to fire only on costs that recur per call / per cold start, keep #4166 as a counter-example, and drop the unverifiable #3944 specifics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: Step 6 — add structural verification for AI-generated code The description-smell table only pre-filters; AI fails structurally. Add six action-forcing checks that each yield a verified/inferred finding: hallucinated -symbol sweep, twin divergence (copy-paste half-adapted), claim/comment↔code + number provenance, safety theater, test-calibrated-to-pass, magic constant without derivation. Motivated by this skill's own fabricated-1.14x P5 bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: close Step-3 routing gaps + add FP self-checks + staleness guards ① Route D6/D7 (fake fns), D8 (contiguous), D9 (int32), D4 (invariant reversal), B7 (over-conservative assert), F1 (double HBM) into Step 3 — they were only reachable by scanning all of Section D. ② Add FP self-checks to B2 (tl.load: is the dim provably aligned?) and C2/C3 (search unchanged lines before firing a "new" hardcode), matching C4/D9. ③ Staleness guards on E4 label→model map and P2 production shapes — a stale snapshot produces confidently-wrong recommendations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: fix B6 (API-modification rule) — route it + widen public-API scope B6 (API propagation) was the rule most about API changes yet had NO Step-3 entry, reachable only by scanning Section B. Route it via a new "API signature change" row + the refactor/rename row. Also fix param-removed/rename scope: a public-op param removal/rename breaks cross-repo consumers (ATOM/SGLang/vLLM), not just same-file call sites — widened, and linked to E1/E5 for public contracts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: add FP self-check to B6 rename/param-removed (found by live test on #4227) Live-testing the skill on aiter#4227 showed B6's rename/param-removed rows had no own FP self-check: a rename shielded by a same-named wrapper + fc_name would read as a 🔴 break if followed verbatim (only the global 🔴 gate rescued it). Add a self-check to confirm no compatibility shim (same-named wrapper / alias / fc_name binding pin) before firing, with #4227 as the real non-example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: E5 — reviewer must proactively @-mention the API owner, not wait Strengthen E5's action from passive "get sign-off" to active: the reviewer must post a PR comment @-mentioning the de-facto owner (@valarLip for fused_moe) with a one-line summary + explicit approval request before merge. Finding is unresolved until the owner has been actively pinged and responded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review-pr: reflect that aiter rejects permanent env-var knobs (D2 scope + HK9) aiter maintainers generally reject new permanent env-var flags: AITER_MOE_FORCE_BF16_ACT (#3593) was reverted by #4225. Reconcile with D2 by scoping D2 to a *temporary rollback kill-switch* only, and upgrade HK9 from 📝 "document it" to⚠️ "prefer auto-derivation; permanent knobs face pushback". ATOM unchanged — it manages env vars via atom/envs.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: root <root@hjbog-srdc-24.amd.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
FP4 support for the DeepSeek-V4 CSA compressor path (gfx950): FlyDSL
fused_compress_attnFP4 scatter + thermsnorm_rope_rotate_activation_fp4quant_kvcacheHIP kernel, plus a KV-scale layout fix.Changes
rmsnorm_rope_rotate_activation_fp4quant_kvcache(dsv4_rotate_quant.cu): fused RMSNorm + RoPE + optional Hadamard rotate + FP4 (E2M1) quant + paged KV-cache write.fused_compress_attnFP4 quant scatter (legacy + K-split), unified behind a singlequant_modeselector (none | per_row_fp8 | group_fp8 | fp4);quant/quant_fp4are derived from it.(pos%16)*(kv_block_size/16) + pos/16) so thepa_mqa_logits_fp4reader's packed-dword load lines up. The linear layout mis-read every scale.Tests
test_dsv4_rotate_quant.py(FP4 kvcache, shuffle scale, rotate) andtest_flydsl_compress_attn.py(FP4 shapes).🤖 Generated with Claude Code