Skip to content

minimax fp8 index cache write - #4098

Merged
zufayu merged 6 commits into
mainfrom
ganyi/fp8_index_cache
Jul 13, 2026
Merged

zufayu merged 6 commits into
mainfrom
ganyi/fp8_index_cache

Conversation

@ganyi1996ppo

Copy link
Copy Markdown
Contributor

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Signed-off-by: ganyi <ygan@amd.com>
@ganyi1996ppo
ganyi1996ppo requested review from a team and Copilot July 6, 2026 07:32
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:triton-300x Run an additional Triton test job on MI300X in PRs; main branch always runs both MI35X and MI300X
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: GPT-OSS-120B, DeepSeek-R1-0528, Kimi-K2.5
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 4098 --add-label <label>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the fused fused_qknorm_idxrqknorm sparse-insert path to support writing the index cache using the same cache element type as the KV caches (including FP8 / uint8-backed FP8 storage), and adjusts the Python perf test to allocate and validate the index cache accordingly.

Changes:

  • Update the kernel signature and store path so index_cache is a cache_t* and uses the FP8 cache store helper (unit scale) in the FP8 path.
  • Relax/adjust C++ input validation so FP8 insert mode accepts index_cache as FP8 or uint8 storage (and non-FP8 mode still requires dtype match with qkv).
  • Update the op_test to optionally allocate index_cache with the cache dtype and to use an FP8 unit-scale reference for the per-token FP8 mode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
op_tests/test_fused_qknorm_idxrqknorm.py Allocates index cache with optional fp8/uint8 dtype and updates reference comparisons for fp8 index-cache writes.
csrc/kernels/fused_qknorm_idxrqknorm.cu Switches index cache to use cache element type and stores via FP8-aware cache write helper; updates dtype checks and launch plumbing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread op_tests/test_fused_qknorm_idxrqknorm.py
@zufayu
zufayu requested a review from amd-ruitang3 July 7, 2026 02:59
Comment thread csrc/kernels/fused_qknorm_idxrqknorm.cu Outdated
Comment thread op_tests/test_fused_qknorm_idxrqknorm.py Outdated
Signed-off-by: ganyi <ygan@amd.com>
Signed-off-by: ganyi <ygan@amd.com>
@zufayu
zufayu requested review from zufayu and removed request for amd-ruitang3 July 8, 2026 05:52
@zufayu
zufayu merged commit 75ad905 into main Jul 13, 2026
42 checks passed
@zufayu
zufayu deleted the ganyi/fp8_index_cache branch July 13, 2026 04:34
zufayu added a commit that referenced this pull request Jul 14, 2026
…/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
valarLip pushed a commit that referenced this pull request Jul 15, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants