Skip to content

flydsl attn: cover the D=64/D=128 backward's uncovered issue windows - #512

Open
kyle-256 wants to merge 1 commit into
mainfrom
attn_bwd_optimize
Open

kyle-256 wants to merge 1 commit into
mainfrom
attn_bwd_optimize

Conversation

@kyle-256

@kyle-256 kyle-256 commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

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

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Infra/Build change
  • Code refactoring

Performance only: no API change, no numerical-semantics change, no new public knob.

Changes

dS publish → GEMM2's run

  • g2_fill: hand the dS pack and its LDS publish to GEMM2, emitted between the first
    d-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 -- tile nt's block feeds only tile
    nt's dK MFMA -- giving each block 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: hand 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: carry a deferred GEMM3 across the q-loop trip as well, so the last head's
    dQ 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 were
    the 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 halves
    contract 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 covers
    the 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_kv equals its window, was
    paying most for the slack.
  • FQ_PAIR: walk 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 and the second read hits in L2.

Measurements

MI355X (gfx950), bf16, S=8192, against the merge base 4a96767d. Both arms are loaded as
two 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:

shape config main TF/s this PR TF/s ratio
gpt-oss D=64 causal B4 Hq64 Hkv8 1027 / 1010 1100 / 1070 1.0708 / 1.0602
gpt-oss D=64 W=128 B4 Hq64 Hkv8 312 / 313 334 / 336 1.0714 / 1.0736
llama 70B D=128 causal B2 Hq64 Hkv8 982 / 990 1022 / 1023 1.0409 / 1.0329
llama 70B D=128 causal B1 Hq64 Hkv8 1026 / 998 1060 / 1043 1.0332 / 1.0451
llama 8B D=128 causal B2 Hq32 Hkv8 982 / 975 1048 / 1032 1.0671 / 1.0590
llama 8B D=128 causal B1 Hq32 Hkv8 985 / 994 1056 / 1054 1.0714 / 1.0598
D=128 W=128 B4 Hq64 Hkv8 292 / 291 293 / 291 1.0051 / 1.0008
geomean 1.0568 / 1.0538

Backward FLOPs are 10 * B*Hq*S^2*D times 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:

  • The four D=128 cells gain 3.3-7.1%, which for most of them is within that cell's own
    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.
  • The D=128 windowed cell is reported flat on purpose. On byte-identical source its
    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_d128 with the atomic dQ path off, feeding the
    DRAM-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:

dQ dK dV
D=128 47.8 dB 49.1 dB 49.4 dB
D=64 49.3 dB 48.8 dB 49.0 dB

against a 45 dB floor. dQ remains atomically accumulated and therefore
non-deterministic, exactly as before this PR.

Checklist:

  • The functionality is complete
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (n/a -- no doc covers this kernel)
  • My changes generate no new warnings (ruff check and ruff format --check clean; no new
    compiler diagnostics across the JIT builds exercised by the benchmarks above)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

The last two are not checked: this PR adds no test, and I have not run the repository's
test suite locally. Correctness was verified out of tree with the SNR gate above on both
head dims, causal and windowed. Happy to run tests/ before merge if you want that box
ticked.

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.

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.

🟡 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

  • _pair cannot become true for any currently dispatched shape: _fuse_blockkv_for caps D64 at 256 and D128 at 128, so D * block_kv is at most 16384, while _fuse_halves only exceeds one at 32768. Consequently g2_kvfuse/g1_kvfuse and 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 or gate widens q_split for 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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants