Conversation
The gfx950 flash-attention backward leaves work standing in windows where
the matrix pipe is idle: the dS pack and its LDS publish, GEMM3's dQ atomic
burst, GEMM1's A-fragment batch, and the second kv half's operand reads.
None of it can be deleted -- every value descends from the same head-step --
but all of it can be issued somewhere a MFMA run already covers.
Every change here is order-only: the same MFMAs on the same operands in the
same per-accumulator order, 16-bit throughout, and no extra register.
dS publish and the GEMM2 run
* g2_fill hands the pack and the publish to GEMM2, emitted between the
first d-tile's dV and dK groups, so the rest of that run covers them.
* g2_weave splits the parked chain per kv tile -- tile nt's block feeds
only tile nt's dK MFMA -- so each block gets a matrix pair to issue
under. A sched_barrier that lets MFMA, and only MFMA, cross pins the
split the scheduler would otherwise undo.
* g2_hs hands GEMM2's closing d-tiles across the dS fence, where they
cover GEMM3's transpose-read burst; they read neither of its operands.
dQ stores
* g3_st_hs/g3_st_g2/g3_st_w: an undeferred GEMM3 ends its head-step with
an uncovered atomic burst. Hand the stores to the next head-step and
issue them a pair of dQ rows at a time between GEMM2's d-tile groups.
* g3_qcarry carries a deferred GEMM3 across the q-loop trip as well, so
the last head's dQ rides the next trip instead of a bare tail.
reads
* g1_pf issues GEMM1's A fragments from inside the previous q-half's
GEMM2 -- they were the one read family with no prefetch ring.
* g2_kvfuse/g1_kvfuse emit the two kv halves' d-tile loops as one: the
halves contract the same Q/dO fragments, so each is read once and the
MFMA run between two reads doubles.
dispatch
* _a16_qsplit reads the launch plan instead of assuming a D=64 dispatch
covers the machine, and only widens the cut when the GQA group is wide
enough to use it.
* _wsq_ilv grouped bands off a conservative span; use the distance at
which two bands' q BLOCK ranges stop touching. A windowed band, whose
block_kv equals its window, was paying most for the slack.
* FQ_PAIR walks each band phase-shifted by its own index so a band's far
half-tile and the next band's near half-tile share a tile in L2.
Measured same-process paired against this file's parent, on MI355X at
S=8192: gpt-oss D=64 causal and llama D=128 (70B and 8B) both gain, the
sliding-window shapes gain, and gradient SNR holds above the 45 dB floor.
kyle-256
requested review from
RuibinCheung,
Xiaoming-AMD,
wenxie-amd and
xiaobochen-amd
as code owners
September 15, 2026 08:31
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate correctness and dispatch issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Performance-focused scheduling and dispatch optimizations for gfx950 FlashAttention backward kernels targeting D=64/D=128.
Changes:
- Overlaps GEMM work with dS publication, dQ stores, and operand prefetches.
- Adds fused KV-half execution and A-fragment prefetching.
- Refines q-splitting, workspace grouping, pairing, and reduction dispatch.
File summaries
| File | Findings |
|---|---|
primus_turbo/flydsl/attention/flash_attn_bwd.py |
Critical (1 vote): uninitialized dense-path CuSeqKv; unhashable list used as a reduction cache key. Moderate (1 vote): D=64 q_split gate is too broad. Moderate (2 votes): initial GEMM3 sentinel runs against unwritten data. Nit (1 vote): KV-half fusion path is unreachable for current dispatches. |
Review details
Suppressed comments (3)
primus_turbo/flydsl/attention/flash_attn_bwd.py:4124
- This phase shift changes the order in which each dK/dV accumulator receives its q-block contributions. Those are floating-point MFMA accumulations, so addition is non-associative and this can change the stored dK/dV values (and the deterministic q_split=1 result) even though the same operands are used. That contradicts the PR's performance-only/no numerical-semantics claim; preserve the original q order here or explicitly revise and test the numerical contract.
_tp = (_t + _prot) % fx.Index(FQ_PAIR_NX)
primus_turbo/flydsl/attention/flash_attn_bwd.py:5335
_paircannot become true for any currently dispatched shape:_fuse_blockkv_forcaps D64 at 256 and D128 at 128, soD * block_kvis at most 16384, while_fuse_halvesonly exceeds one at 32768. Consequentlyg2_kvfuse/g1_kvfuseand their fused-half loop are dead on the advertised D64/D128 paths, so this optimization adds complexity without affecting the measured kernels. Please either provide a dispatch that can select it under the register/LDS constraints or remove/defer the flags.
g2_kvfuse=_pair,
primus_turbo/flydsl/attention/flash_attn_bwd.py:4522
- The
D == 64 orgate widensq_splitfor every D64 GQA ratio, including MHA and narrow groups, even though the docstring and PR description say the extra cut should be taken only when the GQA group is wide enough to benefit. When the while-condition fits, this doubles the dK/dV slot workspace and fold work without the claimed head-group pressure reduction. Please gate this on the GQA ratio (or provide a measured D64 exception and update the rationale).
if D == 64 or Hq // Hkv >= 8:
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """Unused cu_seqlens argument slot (read only under ``const_expr(varlen)``). | ||
| Left uninitialised: no reader means the fill is pure cost, and it is a whole dispatch | ||
| sitting in front of every backward.""" | ||
| return torch.empty(1, device=device, dtype=torch.int32) |
| block=cfg[0], | ||
| uc=cfg[1], | ||
| vec=cfg[2], | ||
| sub=sub, |
| if const_expr(G3_QCARRY): | ||
| # Priming trip: one q row past the sequence end, so the dQ atomics of the | ||
| # not-yet-written dS slot every first head-step reads all clip on num_records. | ||
| _carry = _carry + [seq_len_q_v] |
This branch has not been deployed
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.
Description
The gfx950 flash-attention backward leaves work standing in windows where the matrix
pipe is idle: the dS pack and its LDS publish, GEMM3's dQ atomic burst, GEMM1's
A-fragment batch, and the second kv half's operand reads. None of it can be deleted --
every value descends from the same head-step -- but all of it can be issued where a
MFMA run already covers it.
This is a performance-only change to
primus_turbo/flydsl/attention/flash_attn_bwd.py.Every change is order-only: the same MFMAs on the same operands in the same
per-accumulator order, 16-bit throughout, no extra register, and no change to what the
op computes or to any public signature. dQ stays on the atomic path it already used, so
its existing non-determinism is unchanged (neither improved nor made worse).
Measured on MI355X against this PR's merge base, two independent runs, geomean over
seven shapes: 1.0568 / 1.0538. Table below.
Fixes # (n/a -- no issue)
Type of change
Changes
dS publish → GEMM2's run
g2_fill: hand the dS pack and its LDS publish to GEMM2, emitted between the firstd-tile's dV and dK groups, so the rest of that run covers them while they still
precede the dK MFMAs that read them.
g2_weave: split the parked chain per kv tile -- tilent's block feeds only tilent's dK MFMA -- giving each block a matrix pair to issue under. Asched_barrierthat lets MFMA, and only MFMA, cross pins the split the scheduler would otherwise undo.
g2_hs: hand GEMM2's closing d-tiles across the dS fence, where they cover GEMM3'stranspose-read burst; they read neither of its operands.
dQ stores
g3_st_hs/g3_st_g2/g3_st_w: an undeferred GEMM3 ends its head-step with anuncovered atomic burst. Hand the stores to the next head-step and issue them a pair of
dQ rows at a time between GEMM2's d-tile groups.
g3_qcarry: carry a deferred GEMM3 across the q-loop trip as well, so the last head'sdQ rides the next trip instead of a bare
[drain, barrier, GEMM3]tail.Reads
g1_pf: issue GEMM1's A fragments from inside the previous q-half's GEMM2 -- they werethe one read family with no prefetch ring, so their batch opened each q-half with the
whole LDS latency and no matrix work in flight.
g2_kvfuse/g1_kvfuse: emit the two kv halves' d-tile loops as one. The halvescontract the same Q/dO transpose fragments, so each is read once and the MFMA run
standing between two reads doubles.
Dispatch / addressing
_a16_qsplit: read the launch plan (B, Skv) instead of assuming a D=64 dispatch coversthe machine, and widen the cut only when the GQA group is wide enough to use it.
_wsq_ilv: band grouping used a conservative span; use the distance at which two bands'q BLOCK ranges stop touching. A windowed band, whose
block_kvequals its window, waspaying most for the slack.
FQ_PAIR: walk each band phase-shifted by its own index, so a band's far half-tile andthe next band's near half-tile share a tile and the second read hits in L2.
Measurements
MI355X (gfx950), bf16, S=8192, against the merge base
4a96767d. Both arms are loaded astwo modules in one process and timed ABBA at rep level, so clock drift and DPM state
divide out; an unmodified tree reads 1.000 on this ruler (A-vs-A over the full table:
1.0010). Two independent runs:
Backward FLOPs are
10 * B*Hq*S^2*Dtimes the fraction of the S^2 block computed(1/2 causal, (W+1)/S windowed); each cell shows run 1 / run 2. The windowed cells' low
TF/s is shape-determined, not a regression -- at W=128 only 1.6% of the S^2 block is
computed, so the kernel is dominated by launch and boundary work.
Two notes on reading the table honestly:
run-to-run residual (2.2-4.9%). Eight of eight readings across two runs are positive,
so the direction is solid, but do not read a single D=128 cell's magnitude as exact.
two arms read up to 1.7% apart, so nothing under 2% on that shape is a measurement. It
is the one configuration that is
_fuse_d128with the atomic dQ path off, feeding theDRAM-bound split-K fold; the two arms allocate their own large dQ partial workspace and
do not land on the same pages.
Correctness
All three gradients against an fp32 eager attention, on the shipped default path:
against a 45 dB floor. dQ remains atomically accumulated and therefore
non-deterministic, exactly as before this PR.
Checklist:
ruff checkandruff format --checkclean; no newcompiler diagnostics across the JIT builds exercised by the benchmarks above)