Fix two latent synchronization bugs in the SM100 DSA backward kernel - #395
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSM100 sparse-attention backward
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReduceWarps
participant tmem_dealloc_barrier
participant ComputeWarp0
participant TMEM
ReduceWarps->>ReduceWarps: Complete final dKV T2R operations
ReduceWarps->>tmem_dealloc_barrier: arrive()
ComputeWarp0->>tmem_dealloc_barrier: arrive_and_wait()
tmem_dealloc_barrier-->>ComputeWarp0: All reduce warps arrived
ComputeWarp0->>TMEM: dealloc_tmem()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/python/fe_api/dsa/test_DSA_sparse_attention_backward.py (1)
209-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the P and dS stage controls independently.
Setting both stages to
2can still pass if one path accidentally uses the other path’s stage count or producer state. Add{P: 2, dS: 1}and{P: 1, dS: 2}cases to verify the stage-specific contract.Proposed coverage
- dq, dkv, d_sink = run({"compute_mma_P_stage": 2, "compute_mma_dS_stage": 2}) - - assert not torch.isnan(dq).any() and not torch.isnan(dkv).any() and not torch.isnan(d_sink).any(), "staged store paths produced NaN gradients" - assert torch.equal(dq, dq_ref), "dq must be bitwise-identical between stage 1 and stage 2" + for overrides in ( + {"compute_mma_P_stage": 2}, + {"compute_mma_dS_stage": 2}, + {"compute_mma_P_stage": 2, "compute_mma_dS_stage": 2}, + ): + dq, dkv, d_sink = run(overrides) + assert not torch.isnan(dq).any() + assert not torch.isnan(dkv).any() + assert not torch.isnan(d_sink).any() + assert torch.equal(dq, dq_ref) + assert rel_l2(dkv, dkv_ref) < 1e-4 + assert rel_l2(d_sink, d_sink_ref) < 1e-4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/python/fe_api/dsa/test_DSA_sparse_attention_backward.py` around lines 209 - 218, Extend the staged backward coverage around run and the existing stage-2 assertions by adding independent cases for compute_mma_P_stage 2 with compute_mma_dS_stage 1, and compute_mma_P_stage 1 with compute_mma_dS_stage 2. Validate each case against the stage-1 baseline using the same NaN, dq bitwise, and dkv/d_sink relative-parity checks, ensuring each stage control is exercised independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/python/fe_api/dsa/test_DSA_sparse_attention_backward.py`:
- Around line 209-218: Extend the staged backward coverage around run and the
existing stage-2 assertions by adding independent cases for compute_mma_P_stage
2 with compute_mma_dS_stage 1, and compute_mma_P_stage 1 with
compute_mma_dS_stage 2. Validate each case against the stage-1 baseline using
the same NaN, dq bitwise, and dkv/d_sink relative-parity checks, ensuring each
stage control is exercised independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8328ecbb-7f61-4f01-8a2a-6094a816f3cc
📒 Files selected for processing (2)
python/cudnn/deepseek_sparse_attention/sparse_attention_backward/dsa_bwd_sm100.pytest/python/fe_api/dsa/test_DSA_sparse_attention_backward.py
The compute->MMA smem handoff for P and dS in dsa_bwd_sm100.py (FlashAttentionDSABackwardSm100) has three latent inconsistencies. They are invisible today because every affected stage count is 1, but they corrupt gradients the moment either compute->MMA pipeline is deepened: 1. P_smem_layout_store_staged is built with self.load_mma_K_stage instead of self.compute_mma_P_stage -- the stage count of the compute_mma_P pipeline that cycles through this buffer, of the MMA-side read view P_smem_layout_staged, and of the sP SharedStorage allocation. 2. dS_smem_layout_store_staged is likewise built with self.load_mma_K_stage instead of self.compute_mma_dS_stage. 3. In compute(), the store-side partitions tRS_sP / tRS_sdS are sliced at the producer state's initial index once, before the tile loop, so the stsm stores always write slot 0 even though producer_state.advance() cycles the mbarrier phases and the MMA consumer reads slots 0..N-1. With, e.g., compute_mma_dS_stage = 2, the MMA warp consumes every odd tile from a smem slot the compute warps never wrote, and dQ/dKV pick up NaNs within one tile pair (repro script in the PR description flips the stage constants on an unpatched tree and shows exactly this; with this fix the staged configurations become bitwise-identical in dq to the stage-1 baseline). Fix: build both store-view layouts with their pipeline's stage count, and select the store slot from the producer state at the two copy sites. The stage == 1 slot-0 path is kept behind cutlass.const_expr, and the default configuration is provably unchanged: the emitted cubin is byte-identical (same sha256) before and after this commit at the committed stage settings. No behavior change at the current all-1 stage settings; this only unblocks future work that deepens the compute->MMA pipelines, for configurations whose deepened SharedStorage still fits the 227 KB smem budget (the 576/512 head-dim variant cannot take both stages to 2 as-is; the existing SharedStorage size assert fires at compile time in that case).
Deepens compute_mma_P_stage / compute_mma_dS_stage to 2 (the smallest configuration that cycles the compute->MMA smem handoff) and requires dq to stay bitwise-identical to the stage-1 baseline; dkv/d_sink are fp32 atomic reductions and are gated on NaN and relative parity instead. On the previous commit's parent this test fails with NaN gradients.
In FlashAttentionDSABackwardSm100, compute warp 0 calls dealloc_tmem as soon as its own compute() work is done. The reduce warps run an independent pipeline (mma_reduce_dKV) and may still be executing their final dKV tcgen05.ld (T2R) out of those TMEM columns at that point: nothing orders the dealloc after the reduce warps' last read. The existing t2r_dKV01/4_done barriers only synchronize the reduce warps with the MMA warp for intra-CTA column reuse, not with compute warp 0. tcgen05.dealloc frees the columns for a successor CTA on the same SM; if that CTA allocates and its MMA starts writing while the predecessor's T2R is still in flight, the drained dKV values are corrupted. The window is the kernel tail (compute finishing its dQ stores while reduce is still draining dKV), so any corruption would be timing-dependent and silent -- the ordering is missing by construction. Fix: a dedicated named barrier (id 9, the first free slot; num_reduce_warps + 1 warps). Each reduce warp arrives (without stalling) after reduce_dKV() returns -- at that point every T2R this warp issued has completed and been fenced: store_dKV executes fence_view_async_tmem_load internally, and the split t2r_dKV call sites fence before their pipeline release. Compute warp 0 arrive-and-waits right before dealloc_tmem. dq stays bitwise-identical (the fix only adds ordering at the kernel tail); dkv/d_sink unchanged within their fp32-atomic run-to-run jitter.
9ae03e5 to
0f23df4
Compare
|
Thanks @Anerudhan! I've rebased onto the latest It was a clean union — kept the existing |
| num_threads=(self.num_reduce_warps + 1) * self.threads_per_warp, | ||
| ) | ||
| # TMEM dealloc (compute warp 0) must be ordered after the reduce | ||
| # warps' final dKV T2R has drained; otherwise a successor CTA can |
There was a problem hiding this comment.
I think there's no successor CTA in our kernel. 1 CTA/SM is in this kernel so that there's no issue even if deallocating tmem early. Could you modify the comments to not make readers confused?
But I agree that we need to add a namedbarrier to ensure the dealloc tmem happens after the store dKV.
There was a problem hiding this comment.
Thanks for the review! You're right that this kernel is 1 CTA/SM, so there is no successor CTA and the old wording was misleading. I've reworded the two comments (the tmem_dealloc_barrier definition and the dealloc_tmem site) to attribute the barrier to the intra-CTA ordering instead: compute warp 0's dealloc_tmem must happen after the reduce warps have drained their final dKV T2R reads (the reads that feed store_dKV), otherwise the dealloc would race those in-flight T2R loads within the same CTA. The successor-CTA framing is removed. The barrier and all logic are unchanged — comment-only in 18c0277.
|
only a small comment, LGTM, thanks a lot. I will keep reviewing the next PR. |
…ot successor-CTA The tmem_dealloc_barrier comment attributed the ordering to a successor CTA re-allocating the TMEM columns. This kernel runs 1 CTA/SM, so there is no successor CTA; the reason the barrier is required is the intra-CTA read-before-dealloc ordering: compute warp 0's dealloc_tmem must happen after the reduce warps have drained their final dKV T2R reads (the reads feeding store_dKV), otherwise the dealloc would race those in-flight T2R loads within the same CTA. Comment-only change; the barrier and all logic are unchanged.
|
@Anerudhan I think we can merge this PR |
|
Thanks @Jie-Fang |
|
@cudnn-ci-bot run |
|
@cudnn-ci-bot run |
|
🚀 Running mirror pipeline Branch: cudnn-gh/pr-395-18c0277 |
Before submitting
pre-commit runand committed any formatting changes. (clang-format skipped — no C/CUDA files; black + black-jupyter pass.)Affected area
FE OSS kernels or CuTeDSL
Summary
dsa_bwd_sm100.py(FlashAttentionDSABackwardSm100) carries two latentsynchronization bugs. Neither changes any current numeric result, but each
corrupts gradients under a configuration the code already anticipates:
Staged compute→MMA store views for P and dS.
P_smem_layout_store_staged/dS_smem_layout_store_stagedare built withself.load_mma_K_stageinstead ofself.compute_mma_P_stage/self.compute_mma_dS_stage, and incompute()the store-side partitionstRS_sP/tRS_sdSare sliced at the producer's initial index once beforethe tile loop, so
stsmalways writes slot 0 whileproducer_state.advance()cycles the mbarrier phases and the MMA consumer reads slots 0..N-1. Dormant
only because every compute→MMA stage count is 1 today; deepening either
pipeline yields silent NaNs.
TMEM dealloc lifetime at the kernel tail.
Compute warp 0 calls
dealloc_tmemas soon as its owncompute()finishes;nothing orders that after the reduce warps' final dKV T2R. The existing
t2r_dKV01/4_donebarriers sync reduce with the MMA warp only — computewarp 0 is in neither. A successor CTA can re-allocate and overwrite the
columns while a T2R is still in flight.
Why
compute_mma_dS_stage = 2the MMA warp consumes odd tiles from asmem slot the compute warps never wrote; dQ/dKV pick up NaNs within one tile
pair. Deepening these pipelines is a natural next step (overlapping
softmax-scale work with the dKV/dQ GEMMs) and hits silent corruption with no
hint the store paths are the cause. The optimize dsa bwd sm100 kernel #318 rewrite additionally hoisted the
store partition out of the loop (item 3 of this bug).
tcgen05.deallocfrees the TMEM columns for a successor CTA on the same SM;if that CTA allocates and its MMA starts writing while the predecessor's T2R
is still in flight, the drained dKV values are silently corrupted. The window
is the kernel tail (compute finishing its dQ stores while reduce is still
draining the last dKV tile) — timing-dependent and silent, i.e. incorrect by
construction even though today's scheduling happens to win the race.
Fixes: (1) build both store-view layouts with their pipeline's stage count and
select the store slot from the producer state, keeping the
stage == 1slot-0path behind
cutlass.const_expr(byte-identical default); (2) a dedicated namedbarrier (id 9) — reduce warps arrive after
reduce_dKV()returns (all their T2Ralready fenced:
store_dKVrunsfence_view_async_tmem_loadinternally, thesplit
t2r_dKVpaths fence at their call sites), compute warp 0 arrive-and-waitsright before
dealloc_tmem.Related issues
None filed; found by code inspection + staged-configuration experiments while
porting downstream optimizations onto the DSA backward.
API and compatibility impact
No functional change to any current result.
byte-identical (same sha256) — zero change for every current config; only
unblocks future work that deepens the compute→MMA pipelines (within the 227 KB
smem budget; the 576/512 variant's existing SharedStorage assert still guards
the over-budget case).
topk=1024, bf16, sink on) on B200 — paired interleaved A/B, 200+ samples/side,
sides swapped; medians within noise, minimums differ 0.006%. dq bitwise;
dkv/d_sink unchanged within fp32-atomic run-to-run jitter.
Testing
B200 (SM100), CUDA 13.3, nvidia-cutlass-dsl[cu13] 4.5.2, torch 2.13.
pre-commit runon both files: black + black-jupyter pass (clang-format n/a).
Bug 1. New L0 regression test
test_DSA_sparse_attention_backward_staged_store(deepens both compute→MMA stages to 2, requires dq bitwise == stage-1 baseline):
1 passed; fails with NaN on the unpatched parent. Standalone repro on unpatchedfe7c8b0(S=2048 topk=512 H=64 D=512 bf16):Patched tree: every staged variant dq-bitwise vs stock, dkv/d_sink within
fp32-atomic run-to-run jitter (control in script). Default-config cubin sha256
identical before/after.
Bug 2. dq bitwise; paired A/B no measurable cost (above).
Pre-existing
test_DSA_sparse_attention_backward_wrapper[...512...]cases OOM onour heavily shared B200 (fp32 reference ~1 GiB vs <1 GiB free) identically on the
unpatched base — unrelated to this change; maintainer CI with a dedicated GPU
should pass them (stage-1 cubin is identical).
Repro script (self-contained; flips stage constants via monkeypatch, no tree edit)
```python #!/usr/bin/env python3 """Repro for the latent staged-store bugs in the SM100 DSA backward kernel (cudnn-frontend, python/cudnn/deepseek_sparse_attention/sparse_attention_backward/dsa_bwd_sm100.py).Self-contained: needs only torch + an importable
cudnnpython package withthe DSA CuTe-DSL path (no other files). It does NOT modify the installed
tree: the stage constants are flipped by monkeypatching _setup_attributes,
which is exactly what future in-tree staging work would do by editing them.
variant 'stock' : stages as committed (all 1) -> reference
variant 'ds2' : compute_mma_dS_stage = 2
variant 'p2' : compute_mma_P_stage = 2
variant 'ds2p2' : both = 2
On an UNFIXED tree: ds2/p2/ds2p2 produce NaNs (dq and/or dkv) -> exit 2
On a FIXED tree : dq bitwise == stock for every variant, dkv /
d_sink within fp32-atomic run-to-run jitter -> exit 0
Judging criteria: dq is TMA-stored with a single writer per element, hence
deterministic -> bitwise gate. dkv and d_sink are fp32 atomic reductions and
are NOT bitwise-stable run to run even at fixed config -> gated on NaN count
and relative L2 against a measured stock-rerun jitter control.
Usage: CUDA_VISIBLE_DEVICES= python3 repro_staged_store_standalone.py
"""
import sys
import torch
from cudnn import DSA
import cudnn.deepseek_sparse_attention.sparse_attention_backward.dsa_bwd_sm100 as kmod
import cudnn.deepseek_sparse_attention.sparse_attention_backward._interface_sm100 as imod
import cudnn.deepseek_sparse_attention.sparse_attention_backward.api as amod
S, TOPK, H, D = 2048, 512, 64, 512 # topk/64 = 8 tiles: pipelines really cycle
DTYPE = torch.bfloat16
def make_case(seed=0):
g = torch.Generator(device="cuda").manual_seed(seed)
q = torch.randn(S, H, D, device="cuda", dtype=torch.float32, generator=g).to(DTYPE) / 10
kv = torch.randn(S, D, device="cuda", dtype=torch.float32, generator=g).to(DTYPE) / 10
dout = torch.randn(S, H, D, device="cuda", dtype=torch.float32, generator=g).to(DTYPE) / 10
sink = torch.linspace(-2.0, 2.0, H, device="cuda", dtype=torch.float32)
idx = torch.empty(S, TOPK, device="cuda", dtype=torch.int32)
for i in range(0, S, 1024):
r = torch.rand(min(1024, S - i), S, device="cuda", generator=g)
idx[i : i + r.shape[0]] = r.argsort(dim=-1)[:, :TOPK].to(torch.int32)
tl = torch.full((S,), TOPK, dtype=torch.int32, device="cuda")
scale = D ** -0.5
# chunked reference forward (produces out + KV-only natural-log lse)
out = torch.empty(S, H, D, dtype=DTYPE, device="cuda")
lse = torch.empty(S, H, dtype=torch.float32, device="cuda")
for i in range(0, S, 128):
kv_g = kv[idx[i : i + 128].long()].float()
sc = torch.einsum("chd,ckd->chk", q[i : i + 128].float(), kv_g) * scale
l = torch.logsumexp(sc, dim=-1)
p = torch.exp(sc - torch.logaddexp(l, sink.view(1, H)).unsqueeze(-1))
out[i : i + 128] = torch.einsum("chk,ckv->chv", p, kv_g).to(DTYPE)
lse[i : i + 128] = l
return q, kv, sink, dout, idx, tl, out, lse, scale
def main():
q, kv, sink, dout, idx, tl, out, lse, scale = make_case()
orig = kmod.FlashAttentionDSABackwardSm100._setup_attributes
if name == "main":
main()