Skip to content

fix(fused_moe): pad MXFP4 A4W4 MoE sort extent to a block_size multiple - #5573

Merged
zufayu merged 3 commits into
ROCm:mainfrom
zejunchen-zejun:zejun/fix-mxfp4-a4w4-gemm2-oob
Sep 16, 2026
Merged

zufayu merged 3 commits into
ROCm:mainfrom
zejunchen-zejun:zejun/fix-mxfp4-a4w4-gemm2-oob

Conversation

@zejunchen-zejun

Copy link
Copy Markdown
Contributor

The flydsl MXFP4 A4W4 atomic MoE gemm2 kernel hard-faults with a HIP "illegal memory access" under load. Root cause is a broken block-alignment invariant between the sort stage and the gemm kernel, not a bug in the arithmetic:

  • gemm2s atomic epilog runs the NON-persistent grid and issues _issue_all_a_loads() unconditionally for every grid block (incl. the trailing padding block) BEFORE the if bx_i32 < bound guard, relying on the A buffer descriptor to clamp. The descriptor is sized to max_m_blocks*BM rows (max_m_blocks = ceil(max_sorted/BM)).
  • The A buffer (inter_sorted_quant) is allocated with exactly max_sorted rows. When max_sorted is not a multiple of BM the descriptor over-reads ceil(max_sorted/BM)*BM - max_sorted rows and faults on an unmapped page.

_adaptive_moe_sort always rounds max_sorted up to a BM multiple, so the unconditional load is safe there. PR #4526 added _aux_uses_opus(), which diverts the A4W4 atomic path to the Opus sort whose max_num_tokens_padded = topk_ids.numel() + num_experts*block_size - topk is NOT a block multiple, breaking the invariant.

Fix: round max_num_tokens_padded up to max_num_m_blocks*block_size in both Opus/flydsl sort sizing sites, matching what _adaptive_moe_sort maintains. max_num_m_blocks is unchanged; only the row extent is enlarged.

Validated on Kimi-K2.7-Code-MXFP4 (-tp 4): full gsm8k sweep 1319/1319 clean (0 illegal-memory-access), accuracy 0.9484 flexible / 0.9477 strict.

Adds op_tests/flydsl_tests/test_mxfp4_a4w4_moe_oob.py, a standalone, model-free repro with three tests:

  • test_moe_sorting_opus_row_extent_block_aligned - drives the production sort (output_aux=AUX_SORT_OPUS) and asserts the sorted extent is a block multiple (deterministic; fails pre-fix, passes post-fix).
  • test_mxfp4_gemm2_descriptor_fits_allocation - the same invariant as pure arithmetic, naming the exact 21 rows over-read at the K2.7 fault shape.
  • test_mxfp4_a4w4_gemm2_illegal_access - runs the real production kernel flydsl_mxfp4_gemm2(atomic=True) at the fault shape with the A buffer against a VMM guard page, reproducing the actual HIP illegal memory access in a subprocess (SIGABRT pre-fix, clean exit post-fix).

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

The flydsl MXFP4 A4W4 atomic MoE gemm2 kernel hard-faults with a HIP
"illegal memory access" under load. Root cause is a broken block-alignment
invariant between the sort stage and the gemm kernel, not a bug in the
arithmetic:

  * gemm2s atomic epilog runs the NON-persistent grid and issues
    _issue_all_a_loads() unconditionally for every grid block (incl. the
    trailing padding block) BEFORE the `if bx_i32 < bound` guard, relying on
    the A buffer descriptor to clamp. The descriptor is sized to
    max_m_blocks*BM rows (max_m_blocks = ceil(max_sorted/BM)).
  * The A buffer (inter_sorted_quant) is allocated with exactly max_sorted
    rows. When max_sorted is not a multiple of BM the descriptor over-reads
    ceil(max_sorted/BM)*BM - max_sorted rows and faults on an unmapped page.

_adaptive_moe_sort always rounds max_sorted up to a BM multiple, so the
unconditional load is safe there. PR ROCm#4526 added _aux_uses_opus(), which
diverts the A4W4 atomic path to the Opus sort whose
max_num_tokens_padded = topk_ids.numel() + num_experts*block_size - topk
is NOT a block multiple, breaking the invariant.

Fix: round max_num_tokens_padded up to max_num_m_blocks*block_size in both
Opus/flydsl sort sizing sites, matching what _adaptive_moe_sort maintains.
max_num_m_blocks is unchanged; only the row extent is enlarged.

Validated on Kimi-K2.7-Code-MXFP4 (-tp 4): full gsm8k sweep 1319/1319 clean
(0 illegal-memory-access), accuracy 0.9484 flexible / 0.9477 strict.

Adds op_tests/flydsl_tests/test_mxfp4_a4w4_moe_oob.py, a standalone,
model-free repro with three tests:
  * test_moe_sorting_opus_row_extent_block_aligned - drives the production
    sort (output_aux=AUX_SORT_OPUS) and asserts the sorted extent is a
    block multiple (deterministic; fails pre-fix, passes post-fix).
  * test_mxfp4_gemm2_descriptor_fits_allocation - the same invariant as pure
    arithmetic, naming the exact 21 rows over-read at the K2.7 fault shape.
  * test_mxfp4_a4w4_gemm2_illegal_access - runs the real production kernel
    flydsl_mxfp4_gemm2(atomic=True) at the fault shape with the A buffer
    against a VMM guard page, reproducing the actual HIP illegal memory
    access in a subprocess (SIGABRT pre-fix, clean exit post-fix).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

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:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs; main branch always runs both MI35X and MI300X
multigpu Aiter multi-GPU tests on the 8-GPU runner
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 5573 --add-label <label>

PR title tags & labels:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title and as PR labels automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf], op tags like [MLA], and human labels (ci:*) are left untouched. Add the no-auto-title label to opt this PR out.

Run the allocation invariant from the top-level MoE test so standard Aiter CI collects it. Keep the VMM guard-page check behind gfx950, CuPy, and HIP VMM capability checks, and retain a focused CLI entry for local reproduction.
Enforce the block-aligned allocation contract in the existing MoE sorting reference and output comparisons. Add a focused AUX_SORT_OPUS case across supported block sizes, replacing the bespoke VMM and subprocess machinery in test_moe_2stage.
@fsx950223
fsx950223 marked this pull request as ready for review September 16, 2026 07:28
@fsx950223
fsx950223 requested a review from a team September 16, 2026 07:28
@zufayu

zufayu commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Advisory review (static + hand-run; not a merge gate). Validation/Perf ran no GPU stage — reasons are on their lines below. Findings tagged [verified] are traced to code/repro.

*Rounds the Opus/FlyDSL MoE sort's padded row extent in aiter/fused_moe.py up to a block_size multiple so the MXFP4 A4W4 atomic GEMM2's A-buffer descriptor (sized ceil(rows/BM)BM rows) never spans more rows than the allocation, plus two regression tests in op_tests/test_moe_2stage.py.

Review (advisory): 🔴 HIGH RISK
Validation (deterministic): NOT RUN — auto-validation disabled (REVIEW_AUTO_VALIDATE=0) and this container's installed aiter cannot import (GLIBCXX_3.4.31 missing for module_aiter_core.so), so the selected target op_tests/test_moe_2stage.py never ran; environment gap, not a PR defect
Perf (advisory): NOT RUN — same environment gap, the perftest harness in op_tests/test_moe_2stage.py was never timed base-vs-head; the change is allocation-only, extent growth bounded below one block of rows (21 rows at the K2.7 fault shape M=1172/E=385/topk=9/BM=32)

🔴 [verified] aiter/fused_moe.py sits on the serving path of every downstream MoE model and this diff changes its sort output extent for all of them, yet the PR carries no ci:* label, so the Atom/SGLang/vLLM jobs — the only CI that runs Kimi-K2.7-Code-MXFP4 and DeepSeek-R1-MXFP4 over this exact sort path — skip by default and a downstream break would surface only after merge. Author must add ci:all (or at least ci:sglang + ci:vllm) and require it green before merge.
🔴 [verified] aiter/fused_moe.py is a Tier-2 downstream contract whose observable behavior changes for every caller (returned sorted_ids/sorted_weights extents become block_size multiples on the default path, not arch-gated), and the PR has zero reviews — a fused_moe.py contract change merged on CI plus one approval is how #3593 got reverted within the hour. Reviewer must @-mention the de-facto owner of aiter/fused_moe.py (git top-committer lalala-sh; Lingpeng Jin is the active revert gatekeeper on this file) in a PR comment requesting explicit sign-off before merge.
⚠️ [verified] test_mxfp4_a4w4_gemm2_guard_page in op_tests/test_moe_2stage.py — the only test that reproduces the actual HIP illegal memory access — silently returns unless cupy is installed, and cupy appears nowhere in aiter's requirements or CI workflows, so wherever it is absent the crash reproducer logs one info line and skips while only the extent test runs. Author must confirm cupy is present in the gfx950 CI image, or make the skip loud (explicit skip-mark) so the repro's absence is visible.
📝 [verified] The PR body still describes op_tests/flydsl_tests/test_mxfp4_a4w4_moe_oob.py with three named tests, but the diff ships two differently-named tests (test_mxfp4_a4w4_sort_extent_alignment, test_mxfp4_a4w4_gemm2_guard_page) inside op_tests/test_moe_2stage.py — a reviewer following the description looks for a file that does not exist. Author must update the PR description's test inventory to match the shipped location and names.

@zufayu
zufayu merged commit 22d2c7c into ROCm:main Sep 16, 2026
84 checks passed
@zufayu

zufayu commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Advisory review (static + hand-run; not a merge gate). Validation/Perf ran no GPU stage — reasons are on their lines below. Findings tagged [verified] are traced to code/repro.

*Rounds the Opus/FlyDSL MoE sort's padded row extent in aiter/fused_moe.py up to a block_size multiple so the MXFP4 A4W4 atomic GEMM2's A-buffer descriptor (sized ceil(rows/BM)BM rows) never spans more rows than the allocation, plus two regression tests in op_tests/test_moe_2stage.py.

Review (advisory): 🔴 HIGH RISK
Validation (deterministic): NOT RUN — auto-validation disabled (REVIEW_AUTO_VALIDATE=0) and this container's installed aiter cannot import (GLIBCXX_3.4.31 missing for module_aiter_core.so), so the selected target op_tests/test_moe_2stage.py never ran; environment gap, not a PR defect
Perf (advisory): NOT RUN — same environment gap, the perftest harness in op_tests/test_moe_2stage.py was never timed base-vs-head; the change is allocation-only, extent growth bounded below one block of rows (21 rows at the K2.7 fault shape M=1172/E=385/topk=9/BM=32)

🔴 [verified] aiter/fused_moe.py sits on the serving path of every downstream MoE model and this diff changes its sort output extent for all of them, yet the PR carries no ci:* label, so the Atom/SGLang/vLLM jobs — the only CI that runs Kimi-K2.7-Code-MXFP4 and DeepSeek-R1-MXFP4 over this exact sort path — skip by default and a downstream break would surface only after merge. Author must add ci:all (or at least ci:sglang + ci:vllm) and require it green before merge.
🔴 [verified] aiter/fused_moe.py is a Tier-2 downstream contract whose observable behavior changes for every caller (returned sorted_ids/sorted_weights extents become block_size multiples on the default path, not arch-gated), and the PR has zero reviews — a fused_moe.py contract change merged on CI plus one approval is how #3593 got reverted within the hour. Reviewer must @-mention the de-facto owner of aiter/fused_moe.py (git top-committer lalala-sh; Lingpeng Jin is the active revert gatekeeper on this file) in a PR comment requesting explicit sign-off before merge.
⚠️ [verified] test_mxfp4_a4w4_gemm2_guard_page in op_tests/test_moe_2stage.py — the only test that reproduces the actual HIP illegal memory access — silently returns unless cupy is installed, and cupy appears nowhere in aiter's requirements or CI workflows, so wherever it is absent the crash reproducer logs one info line and skips while only the extent test runs. Author must confirm cupy is present in the gfx950 CI image, or make the skip loud (explicit skip-mark) so the repro's absence is visible.
📝 [verified] The PR body still describes op_tests/flydsl_tests/test_mxfp4_a4w4_moe_oob.py with three named tests, but the diff ships two differently-named tests (test_mxfp4_a4w4_sort_extent_alignment, test_mxfp4_a4w4_gemm2_guard_page) inside op_tests/test_moe_2stage.py — a reviewer following the description looks for a file that does not exist. Author must update the PR description's test inventory to match the shipped location and names.

vgokhale added a commit that referenced this pull request Sep 17, 2026
…ing fixes, fp8 MQA logits split-k and DSv4 tunings (#5573, #5295, #5558, #5603, #5627, #5485) (#5638)

Cherry-picks six already-merged `main` PRs onto `release/v0.1.22` for the `v0.1.22.post1` post release.

| PR | `main` commit | Backport commit | What |
|---|---|---|---|
| #5485 | `9252f4672` | `155534984` | Extend the DeepSeek-V4 a8w8 blockscale GEMM tunings for gfx950 (tuning CSV only) |
| #5573 | `22d2c7c91` | `6b23ba866` | Pad the MXFP4 A4W4 MoE sort extent to a block_size multiple (fixes a HIP illegal memory access) |
| #5295 | `972c8e1fd` | `7d68b0edb` | Skip invalid expert IDs in MoE sorting |
| #5558 | `3fdfca11e` | `dd83a9d17` | Fix MoE routing kernel compile failure |
| #5603 | `a84bd368c` | `a96461997` | Add split-k support for fp8 MQA logits on gfx950 |
| #5627 | `f5ed7dc54` | `a41214712` | Follow-up to #5603: drop chunking when summation folding is unavailable (fixes Triton 3.6 compile) |

Original PRs:
- #5485: #5485
- #5573: #5573
- #5295: #5295
- #5558: #5558
- #5603: #5603
- #5627: #5627

To be published as `v0.1.22.post1` once merged (tag on the merge commit, release automation builds the wheel set).
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