Repository navigation
1,Balance the prologue's extra wave_m == 1 barrier so both groups' ep… - #1306
Merged
Merged
Conversation
…ilogues overlap (helps small shapes) 2,port aiter pr6368 to fix race issue
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-wave synchronization changes need device validation of the K=256 zero-main-loop path before safe approval.
1 open issue
What changed in this PR
Updates the MXFP8 eight-wave GEMM kernel’s synchronization to address shared-memory races and improve small-shape epilogue overlap.
Changes:
- Waits for shared-memory reads before the refill barrier.
- Drains outstanding loads at the tail.
- Balances the final barrier between wave groups.
| File | Description |
|---|---|
| kernels/gemm/mxfp8_gemm_8wave.py | Adjusts waits and barriers in the GEMM pipeline. |
Review effort: Balanced
| a0_frag = a_s2r.load(a_cur0) | ||
| rocdl.s_barrier() | ||
| # Drain the last tile's loads: its B0/A0 are read before the final wait. | ||
| wait_barrier(0) |
solinzby1
force-pushed
the
mxfp8_perf
branch
2 times, most recently
from
October 20, 2026 02:02
7b8b7db to
6fc2ca5
Compare
…ogues overlap (helps small shapes)
coderfeli
approved these changes
Oct 20, 2026
yangyuhuiling
pushed a commit
to yangyuhuiling/aiter
that referenced
this pull request
Oct 20, 2026
…efore the epilogue The prologue's `if wave_m == 1: s_barrier()` leaves waves 4-7 one barrier ahead in count, and the last unconditional s_barrier only paired with their previous one, so waves 4-7 sat in a barrier until waves 0-3 had stored and exited: the two groups' epilogues ran back to back. Issue the final barrier on wave_m == 0 only, as ROCm/FlyDSL#1306 does for the mxfp8 kernel, so the counts match and both epilogues overlap. Kept unconditional at BLOCK_N=512: that tile is already at 256 VGPRs, the branch raises spills from 4 to 11, and all three tuned 128x512 shapes measured 2.4-2.7% slower with it. The 256-wide tiles have headroom (256x256: 252 VGPRs either way; 128x256: 152 -> 156, no spills). MI355X, FlyDSL 0.4.0, 16 tuned shapes at their tuned tile/wpe/xcd, 7 interleaved rounds: geomean -2.1%, up to -6.1% (1472x7168x1536); large-M multi-wave shapes -0.7% to -2.1%. 18 shapes incl. 4096x7168x256 and 32768x7168x1792 stay 0/8 wrong with NaN-poisoned output. Co-Authored-By: Claude <noreply@anthropic.com>
coderfeli
pushed a commit
to ROCm/aiter
that referenced
this pull request
Oct 26, 2026
…le lands (#6368) * [FlyDSL] [gfx950] Fix 8-wave a8w8 GEMM dropping LDS writes at the K-step barrier The barrier that ends each main-loop K step admits `2 * N_LDS_STEPS_A + N_LDS_STEPS_B` outstanding global->LDS loads. That is more than the step can have in flight and still be safe: the barrier passes while writes the next step reads have not landed, and the next step consumes stale LDS. The result is a few wrong rows, not a NaN -- rel_l2 lands around 1e-3, close enough to fp8 quantisation noise to go unnoticed. Whether it fires depends on how the waves interleave, so a shape can be clean on one launch and wrong on the next, and larger grids hit it more often. Shapes in the tuned configs are affected. `M=32768 N=7168 K=1792` on the 256x256 tile is wrong on 4 of 4 runs, and `M=3456 N=7168 K=4224` on 128x256 on 1 of 4. Lowering the count to `N_LDS_STEPS_A + N_LDS_STEPS_B - 1` clears it: both shapes above are then correct and bit-identical across runs. A sweep of the 128x256 tile (where the two counts are 4 and 2) shows 4 and 3 corrupting and 2 and 0 clean, so this is the loosest count that is safe there. Waiting on more loads costs throughput, mostly on 128x256 and mostly at small grids where the K loop is short enough for the barrier to matter: tile grid before after 128x256 168 55.6us 65.0us +16.9% 128x256 224 60.6us 67.8us +11.9% 128x256 243 64.9us 68.0us +4.8% 128x256 252 63.7us 67.9us +6.6% 256x256 144 84.7us 87.0us +2.7% 256x256 363 167.8us 177.3us +5.7% 256x256 495 204.9us 204.7us -0.1% 256x256 3584 451.7us 479.0us +6.0% Measured on MI355X, min of per-arm medians over two alternating passes. Recovering this would need a tighter count per tile rather than one formula; correctness comes first here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [FlyDSL] [gfx950] Note the divergent prologue barrier in the 8-wave GEMM The prologue's `if wave_m == 1: rocdl.s_barrier()` is executed by waves 4-7 only. Waves 0-3 skip it and reach the next barrier instead, so the hardware pairs the two groups one barrier apart and keeps them that way for the rest of the kernel. Documented rather than changed: making it unconditional while keeping the old wait count still corrupts, and with the corrected count both forms are clean, so it is not what this PR is fixing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [FlyDSL] [gfx950] Correct the note on the prologue's half-wave barrier The previous commit read the divergent `if wave_m == 1: s_barrier()` as a bug in itself. It is not: it deliberately puts waves 0-3 one phase ahead, which is the stagger the double-buffered main loop is built around. Making it unconditional destroys that and fixes nothing. What is missing is its counterpart. The stagger is never closed, so the two halves are still a barrier apart when the epilogue starts. gcnasm closes it before the same epilogue (opus_gemm_a2a_lsa quad-subtile template) and so does mori's fused copy of this pipeline, with `if wave_m == 0: rocdl.s_barrier()`. Still a note rather than a change: nothing follows store_c here, so the imbalance cannot be observed today. It stops being unobservable the moment anything is appended to the epilogue -- a `wait_barrier(0)` placed after store_c would rendezvous waves 0-3 with waves 4-7 still one barrier back and before their stores, rather than meaning what it says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Move the fix into the tail, where the dependency is Tightening the main loop's end-of-step wait from 2A+B to A+B-1 was the wrong place for it, for two reasons. It does not cover every legal K. At K=256, K_ITERS is 2 and the main loop body runs zero times, so the changed line never executes: M=4096 N=7168 K=256 tile=256x256 is still wrong 6 runs out of 6. And it pays per K step for a dependency the tail owns. The peeled steps pre-read the last K step's B0 (end of step K_ITERS-2) and A0 (top of step K_ITERS-1), both before that step's wait_barrier(0). Tightening the main loop only made those tiles land in LDS earlier, which is why it happened to fix the long-K shapes. So: restore the main loop's original count and drain the final-tile producers in the tail, where the pre-reads are. The last step's wait stays -- it still covers the final A1, which is issued after it. Also order the WAR on B0 explicitly. The leading group refills that slot one stage after the barrier the lagging group's S0 pairs with, and LDS reads retire under LGKM, which this kernel's VM waits say nothing about. read_done_barrier() is lgkmcnt(0) + s_barrier. This one is a statically found gap: the tail drain alone already clears every shape here, so it is not separately demonstrated -- it costs nothing measurable and writes the dependency down. On 8x MI355X, 8 runs per shape with NaN-poisoned output, all clean including the two new K=256/512 entries. At 32768x7168x1792 the main loop's restored count is worth 6%: 445.2us against 476.3us for A+B-1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [FlyDSL] [gfx950] Trim comments in the 8-wave GEMM sync fix Review asked for shorter comments. Each added block is now one line, the wait_barrier docstring is dropped, and the epilogue TODO about the unclosed half-wave stagger is removed -- it is recorded in the PR description. No code change. Co-Authored-By: Claude <noreply@anthropic.com> * [FlyDSL] [gfx950] Drop the dedicated 8-wave GEMM test Review preferred not adding a test file per fix. The existing op_tests/test_gemm_a8w8.py already runs every tuned flydsl row through gemm_a8w8_bpreshuffle, which is the only way this kernel is reached on gfx950 (untuned shapes fall back to CK). On 32768x7168x1792 its "ck bpreshuffle err" reads 0.05-0.09% before this fix and 0 after. Co-Authored-By: Claude <noreply@anthropic.com> * [FlyDSL] [gfx950] Use rocdl.s_waitcnt instead of inline asm for the S0 read drain Replace read_done_barrier()'s inline asm with rocdl.s_waitcnt(lgkmcnt=0) + rocdl.s_barrier(), the form FlyDSL#1306 uses for the same fix. Same 54 "s_waitcnt lgkmcnt(0); s_barrier" pairs at K=7168 and the same VGPR/SGPR counts, but 212 (256x256) / 106 (128x256) fewer s_waitcnt overall: the inline asm was opaque to SIInsertWaitcnts, which then re-inserted lgkmcnt waits it could not prove were already satisfied. MI355X, FlyDSL 0.4.0: 4096x7168x256 and 32768x7168x1792 are 8/8 wrong on main and 0/8 with this change, as with the inline asm; timing is within noise of the inline asm on six shapes. Co-Authored-By: Claude <noreply@anthropic.com> * [FlyDSL] [gfx950] Close the 8-wave GEMM's half-wave barrier stagger before the epilogue The prologue's `if wave_m == 1: s_barrier()` leaves waves 4-7 one barrier ahead in count, and the last unconditional s_barrier only paired with their previous one, so waves 4-7 sat in a barrier until waves 0-3 had stored and exited: the two groups' epilogues ran back to back. Issue the final barrier on wave_m == 0 only, as ROCm/FlyDSL#1306 does for the mxfp8 kernel, so the counts match and both epilogues overlap. Kept unconditional at BLOCK_N=512: that tile is already at 256 VGPRs, the branch raises spills from 4 to 11, and all three tuned 128x512 shapes measured 2.4-2.7% slower with it. The 256-wide tiles have headroom (256x256: 252 VGPRs either way; 128x256: 152 -> 156, no spills). MI355X, FlyDSL 0.4.0, 16 tuned shapes at their tuned tile/wpe/xcd, 7 interleaved rounds: geomean -2.1%, up to -6.1% (1472x7168x1536); large-M multi-wave shapes -0.7% to -2.1%. 18 shapes incl. 4096x7168x256 and 32768x7168x1792 stay 0/8 wrong with NaN-poisoned output. Co-Authored-By: Claude <noreply@anthropic.com> * [FlyDSL] [gfx950] Balance the epilogue barrier on every tile; drop the added comments Issue the final barrier on wave_m == 0 for every tile, BLOCK_N=512 included, instead of keeping it unconditional there. Remove the comments this PR added. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: xiangch <xiangch@amd.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

…ilogues overlap (helps small shapes)
2,port aiter pr6368 to fix race issue