frost(sdpa): derive THD token capacity from the view's element span (fixes #613) - #706
frost(sdpa): derive THD token capacity from the view's element span (fixes #613)#706vedaanta wants to merge 2 commits into
Conversation
…VIDIA#613) The zero-host-read THD execute (NVIDIA#606/NVIDIA#608) derives the packed token extents host-side as numel() // token_stride. That is wrong on both edges for the buffers real integrations bind: - A non-packed VIEW — a K/V slice of a kv-interleaved [T, 2, H, D] record, the layout torch.nn.attention.varlen users produce by slicing a fused KV projection — holds T tokens but only T*H*D of the record's elements, so the derived extent HALVES and the TMA descriptors cut off half the tokens: silently wrong O on every such call (issue NVIDIA#613; also 40 upstream PyTorch test_varlen_attention failures through the python-API integration). - Deriving from the untyped storage instead over-claims into ALLOCATOR SLACK, which is not benign: rows between the real packed total and the extent are masked but still multiplied (P == 0 times V), so they must be FINITE — TMA zero-fill only covers rows at or beyond the extent. A slack row carrying NaN bit patterns poisons whole sequences through 0 * NaN. Fix: capacity = the largest T whose final token's ROW still fits in the buffer's own element SPAN (1 + sum((size-1)*stride)). The span is exact on both edges: flat capacity buffers give exactly their token capacity (no slack), and interleaved/gapped views give exactly T. Every row below the capacity lies in caller-provided finite elements; every row at or beyond it TMA-clips to zeros. One shared helper serves the SM100 f16 path and the SM120/FP8 _cap sites. Verified on SM100 (isolated env): the NVIDIA#613 kv-interleave repro 41% -> 0 mismatches (frost-served); test_sdpa_random_fwd_ragged_L0 5-seed slice 84/84 (no regressions); fp8 THD ragged slice green; the new deterministic regression test (fused-record K/V views vs packed binding, torch.equal) fails on develop and passes with the fix; upstream PyTorch test_varlen_attention returns from 100 pass / 69 fail to its 140 / 29 impl-identity baseline with the torch-ops stack applied on top. Fixes NVIDIA#613. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change adds addressable THD capacity calculation for SM100 and SM120. THD packing uses it for Q/O and K/V extents. Ragged strides use derived generation. A regression test validates interleaved strided K/V views. ChangesTHD capacity correction
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change corrects THD token-capacity handling for packed and interleaved views, with the supplied regression and compatibility checks passing; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The seeded per-tensor token-gap draw (NVIDIA#516) lives in ExecConfig.fill_derived_fields and only fills strides left None — but RandomizationContext, which drives every test_sdpa_random_*_ragged sweep, explicitly assigned packed bshd strides in its ragged branch. Net effect: the randomized ragged fleet has NEVER bound a non-packed THD stride, and for packed buffers the numel()//token_stride capacity heuristic is exact — which is precisely why these sweeps stayed green while issue NVIDIA#613 (interleaved K/V views halving the TMA extent) shipped and had to be found through an external integration. Fix: the ragged branch leaves Q/K/V/O strides None and __call__ ends with fill_derived_fields() — one source of truth for the gap draw and its auto-packed fallbacks (cu / offset-multiplier forms NVIDIA#538, 1-byte dtypes NVIDIA#537). The head_major stats stride and the whole dense branch are untouched. Census over the fwd ragged L0 slice (84 configs): before, 0/84 drew a gap although each config's own rng_geom_seed hand-draws nonzero gaps; after, 84/84 draw gaps and ALL 84 would have failed under the old capacity formula. Verified on SM100 (cuDNN 9.26.0.33, CUDNN_FRONTEND_ENABLE_FROST_ENGINES=1): with the NVIDIA#613 fix the gapped fwd ragged L0 slice passes 84/84 (all frost-served) — with the pre-fix adapter swapped in it fails 80/84, i.e. this wiring alone would have caught NVIDIA#613 the day the heuristic merged. bwd ragged L0 slice 158/158, identical to the unwired control on the same lib (the backend serves every gapped gradient combination); ragged_unified_L1 24/24 and offset_multiplier_unified_L1 24/24 (cu / mult forms stay packed via the existing fallbacks — 20/20 each in the offline census); the stride-override unit test still passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Added bd7a8f2 — The coverage seam. The seeded token-gap draw (#516) lives in The commit makes the ragged branch leave Q/K/V/O strides Red-team evidence (SM100, cuDNN 9.26.0.33,
i.e. this wiring alone would have caught #613 the day the capacity heuristic merged. Post-fix matrix, all green:
Note: the bwd baseline requires a backend with the dSink+ragged-stats fixes (9.26 here); on a 9.24 lib the bwd ragged sweep fails for that unrelated pre-existing backend reason with or without this commit. |
|
Run CI. |
|
Follow-up filed as #718. While reasoning about how much of the capacity contract this PR actually buys, I confirmed the other half is still open: Nothing here changes this PR — the span fix is still needed and still correct, since the clamp only helps tensors that get one. Flagging the linkage so the two are reviewed together. |
|
Correction to my note above: the follow-up I filed as #718 is a duplicate of #624 (same defect, already tracked, with harness hardening in flight as #646). #718 is closed and the analysis is consolidated onto #624. The linkage to this PR is unchanged — #706 guarantees rows below capacity are caller-owned; #624 covers their being initialized. |
) * frost(sdpa): derive THD token capacity from the view's element span (#613) The zero-host-read THD execute (#606/#608) derives the packed token extents host-side as numel() // token_stride. That is wrong on both edges for the buffers real integrations bind: - A non-packed VIEW — a K/V slice of a kv-interleaved [T, 2, H, D] record, the layout torch.nn.attention.varlen users produce by slicing a fused KV projection — holds T tokens but only T*H*D of the record's elements, so the derived extent HALVES and the TMA descriptors cut off half the tokens: silently wrong O on every such call (issue #613; also 40 upstream PyTorch test_varlen_attention failures through the python-API integration). - Deriving from the untyped storage instead over-claims into ALLOCATOR SLACK, which is not benign: rows between the real packed total and the extent are masked but still multiplied (P == 0 times V), so they must be FINITE — TMA zero-fill only covers rows at or beyond the extent. A slack row carrying NaN bit patterns poisons whole sequences through 0 * NaN. Fix: capacity = the largest T whose final token's ROW still fits in the buffer's own element SPAN (1 + sum((size-1)*stride)). The span is exact on both edges: flat capacity buffers give exactly their token capacity (no slack), and interleaved/gapped views give exactly T. Every row below the capacity lies in caller-provided finite elements; every row at or beyond it TMA-clips to zeros. One shared helper serves the SM100 f16 path and the SM120/FP8 _cap sites. Verified on SM100 (isolated env): the #613 kv-interleave repro 41% -> 0 mismatches (frost-served); test_sdpa_random_fwd_ragged_L0 5-seed slice 84/84 (no regressions); fp8 THD ragged slice green; the new deterministic regression test (fused-record K/V views vs packed binding, torch.equal) fails on develop and passes with the fix; upstream PyTorch test_varlen_attention returns from 100 pass / 69 fail to its 140 / 29 impl-identity baseline with the torch-ops stack applied on top. Fixes #613. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(sdpa): actually fuzz ragged token gaps in the randomized sweeps The seeded per-tensor token-gap draw (#516) lives in ExecConfig.fill_derived_fields and only fills strides left None — but RandomizationContext, which drives every test_sdpa_random_*_ragged sweep, explicitly assigned packed bshd strides in its ragged branch. Net effect: the randomized ragged fleet has NEVER bound a non-packed THD stride, and for packed buffers the numel()//token_stride capacity heuristic is exact — which is precisely why these sweeps stayed green while issue #613 (interleaved K/V views halving the TMA extent) shipped and had to be found through an external integration. Fix: the ragged branch leaves Q/K/V/O strides None and __call__ ends with fill_derived_fields() — one source of truth for the gap draw and its auto-packed fallbacks (cu / offset-multiplier forms #538, 1-byte dtypes #537). The head_major stats stride and the whole dense branch are untouched. Census over the fwd ragged L0 slice (84 configs): before, 0/84 drew a gap although each config's own rng_geom_seed hand-draws nonzero gaps; after, 84/84 draw gaps and ALL 84 would have failed under the old capacity formula. Verified on SM100 (cuDNN 9.26.0.33, CUDNN_FRONTEND_ENABLE_FROST_ENGINES=1): with the #613 fix the gapped fwd ragged L0 slice passes 84/84 (all frost-served) — with the pre-fix adapter swapped in it fails 80/84, i.e. this wiring alone would have caught #613 the day the heuristic merged. bwd ragged L0 slice 158/158, identical to the unwired control on the same lib (the backend serves every gapped gradient combination); ragged_unified_L1 24/24 and offset_multiplier_unified_L1 24/24 (cu / mult forms stay packed via the existing fallbacks — 20/20 each in the offline census); the stride-override unit test still passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(sdpa): accept max_total_seq_len_q/kv on the forward SDPA node `sdpa_backward` has taken `max_total_seq_len_q/kv` since cuDNN 9.6; the forward node never did. That asymmetry is the root of a whole bug class. A ragged (THD) graph declares `(B, H, S_max, D)` plus a device-side ragged-offset tensor, so the packed token total is not expressible anywhere in the forward graph — and reading `cu_seqlens[-1]` host-side is exactly the D2H sync the zero-host-read THD execute (#552) exists to eliminate. The FROST forward path therefore has to INFER an upper bound on the token axis from the bound buffers' element span (#613/#706). That bound is memory-safe but loose, and looseness is not benign: rows between the real total and the extent are masked yet still multiplied (`P == 0` times V), so they must be FINITE. A caller that over-allocates and leaves the tail unwritten poisons whole tiles through `0 * NaN` (#624). Every framework already has this number — it is `q.shape[0]` in vLLM, SGLang, TransformerEngine, Megatron-Core, PyTorch and FlashInfer alike — and today it gets thrown away at the graph boundary. This lets callers declare it. - C++: `max_total_seq_len_q/kv` on `SDPA_attributes` with setters and serialization, mirroring `SDPA_backward_attributes`. Frontend-side only: like the backward twin it is never lowered to a backend attribute, so it cannot affect backend validation (#704). - Forward node validation rejects it on a non-ragged layout, mirroring backward's "only supported with packed layout". - pybind: `sdpa(..., max_total_seq_len_q=None, max_total_seq_len_kv=None)`. - FROST forward consumes it: the declared total is min'd against the buffer-derived capacity, so it can only TIGHTEN the extent, never widen it. A stale or wrong value cannot make a launch address memory the caller does not own — it can only make it address less. Both the SM100 f16 and the SM120/FP8 extent sites go through one helper. Effect on #624, measured on SM100 (bf16, cuDNN 9.26.0.33, FROST forced), `seq_lens=[200,150,47]` (total 397) bound into `(640, H, D)` buffers whose `[397, 640)` tail is NaN — only the tail fill differs between runs: undeclared: 201,728 NaNs in O (49.6%) declared: 0 NaNs, bit-identical to the zero-tail run Verified: new L0 regression test (asserts the clamp AND that the undeclared path still reaches the tail, so it tests the clamp rather than a benign shape); dense graph + attribute correctly rejected; the #613 interleaved-KV-views test and the gap-wired ragged L0 slice (84/84, all FROST-served) unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(sdpa): expose max_total_seq_len_q/kv on sdpa_fp8 too Review follow-up. `PyGraph::sdpa_fp8` routes through `sdpa_internal`, so it already builds the same `SDPA_attributes` that now carries the packed totals -- only the entry point was missing them, and it hard-coded `py::none()` at the forwarding call. An FP8 THD caller therefore had no way to declare its totals even though the adapter side (`_thd_declared_total` at the SM100 f16 and SM120/FP8 extent sites) was already wired for them. Adds the two optional arguments to the declaration, the definition, the pybind binding and the docstring, and forwards them instead of `py::none()`. `sdpa_mxfp8` is deliberately left out: it does not go through `sdpa_internal` and builds `SDPA_fp8_attributes`, which has no such field, so covering it means extending that struct as well. Note the reviewer's stated motivation does not actually hold for FP8: the FP8/MXFP8 kernels already clamp their K/V descriptor extents to `cu_k[B]` device-side in `build_thd_meta_o_kv_descs_kernel`, so an unwritten K/V capacity tail is already TMA-unreachable there, and Q is the parallel dimension (a garbage Q row poisons only its own row, which is never stored). The change is still worth making for API symmetry and for exact rather than inferred extents. Test: `test_fp8_thd_declared_totals` runs the THD FP8 path with and without the declaration from the same seed and asserts O is bit-identical, plus the usual accuracy check against the reference. Verified: `test_sdpa_fwd_fp8_sm100.py` 61 passed; f16 THD tests 195 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(sdpa): expose max_total_seq_len_q/kv on sdpa_mxfp8 too Correcting my own note on the previous commit: I claimed `sdpa_mxfp8` was out of scope because it "builds `SDPA_fp8_attributes`, which has no such field". That is wrong — `SDPA_fp8_attributes` is a type ALIAS for `SDPA_attributes` (graph_properties.h), so the field has been there all along and the only gap was the pybind entry point. `sdpa_mxfp8` does not route through `sdpa_internal`, so it needed its own declaration, definition, attribute plumbing, binding and docstring — but no struct change. The MXFP8 forward row serves THD (`thd_d_shapes` covers the d128 kernel), and the adapter side (`_thd_declared_total`) was already shared, so this completes the forward family: `sdpa`, `sdpa_fp8` and `sdpa_mxfp8` all now accept the packed totals. Still missing, and genuinely needing a struct change: the FP8/MXFP8 BACKWARD nodes. `SDPA_fp8_backward_attributes` is a distinct class (not an alias) with no such field, so `sdpa_fp8_backward` / `sdpa_mxfp8_backward` cannot take the totals while plain `sdpa_backward` has since cuDNN 9.6. Tracked separately. Test: `test_mxfp8_thd_declared_totals` runs the MXFP8 THD path with and without the declaration from the same seed and asserts O is bit-identical, plus the usual accuracy and amax checks. Verified: `test_sdpa_fwd_mxfp8_sm100.py` THD selection 10 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Superseded by #740 — closing. #740 was stacked on this branch and merged as a squash (
So nothing here is lost, and #613 stays fixed by the merged code rather than by this PR. |
Problem (issue #613)
The zero-host-read THD execute (#606/#608) derives the packed token extents host-side as
numel() // token_stride. That is wrong on both edges for the buffers real integrations bind:[T, 2, H, D]record — the layouttorch.nn.attention.varlenusers produce by slicing a fused KV projection — holdsTtokens but onlyT*H*Dof the record's elements, so the derived extent halves and the TMA descriptors cut off half the tokens: silently wrong O (41% mismatches on the frost(sdpa): THD zero-host-read extents break non-packed views; unit decode is extent-sensitive (regression from #606, also in #608) #613 repro). Through the PyTorch python-API integration this now breaks 40 upstreamtest_varlen_attentioncases, because the engines recently started claiming tiny-window THD configs they previously declined.P(=0) × V), so they must be finite; TMA zero-fill only covers rows at or beyond the extent. One slack row carrying NaN bit patterns poisons whole sequences through0 × NaN(reproduced: batch-wide NaNs in the ragged sweeps).Fix
Capacity = the largest
Twhose final token's row still fits in the buffer's own element span (1 + Σ (size−1)·stride). The span is exact on both edges:T(the last token needs only its own row footprint, not a full record span).Every row below the capacity lies in caller-provided finite elements; every row at or beyond it TMA-clips to zeros. One shared
_thd_capacityhelper serves the SM100 f16 sites and the SM120/FP8_capsites (packed contract included).Verification (SM100, isolated worktree + venv, no shared JIT cache)
test_repro, seeded)torch.equal)test_sdpa_random_fwd_ragged_L05-seed slice (84 tests)test_varlen_attention(with the torch-ops stack on top)The regression test lives next to the #606 suite (
test_dsl_sm100_thd_interleaved_kv_views) and encodes both failure modes in its docstring.Fixes #613.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests