DSA indexer_top_k: exclude OOB tile-fill lanes from radix selection - #445
DSA indexer_top_k: exclude OOB tile-fill lanes from radix selection#445JackRao123 wants to merge 1 commit into
Conversation
The CuTe-DSL radix top-k reads each row in fixed-width vector tiles and fills the final partial tile's out-of-bounds lanes with -inf so the predicated copy is safe, but the histogram and candidate-collection loops then iterate every fragment lane without a bounds check, counting those phantom -inf lanes as real elements. This is harmless while the row's top-k threshold sits above the phantom bin: phantom -inf lanes sort last and are dropped. But the coarse radix bin is derived from the fp16 conversion of the fp32 score (to_coarse_key), so every score below fp16's -65504 minimum collapses into the fp16 -inf bin -- the same bin the phantom lanes occupy. When a row's top-k threshold lands inside that bin (fewer than top_k values above ~-65504), the threshold bin's candidate list becomes real_count + up-to-tile-width phantom lanes. In the large_occupancy compile (>148 rows) the per-row candidate buffers (512-entry smem + num_cols gmem) overflow, producing out-of-bounds shared/global writes and cudaErrorIllegalAddress; in any configuration phantom lanes can be selected as winners, yielding silently out-of-range indices. Observed in production on a GLM-5.2 context-parallel (CP32) training run: deterministic, data-dependent illegal memory access (Xid 43) on B200 and B300, reproduced from the captured score tensor; isolated via CUDA_LAUNCH_BLOCKING and compute-sanitizer (invalid 2-byte shared writes); trigger confirmed as the value distribution, not the shape. Fix: skip lanes whose tile column coordinate is out of bounds in the three vectorized histogram/collection loops, reusing the same aligned_size bound the predicated copy already computes. The scalar prologue/leftover loops are exact-bounded and unchanged. Testing: new L0 regression test test_DSA_indexer_top_k_oob_tile_lanes deterministically drives a row into the flood regime in both compile-time variants (phantom-dominated candidate lists with identical and with distinct collapsed values). On v1.26.0 with this patch applied (B200/B300, SM90+/SM100): the previously-crashing inputs complete with in-range indices, compute-sanitizer reports no invalid accesses, and 12/12 randomized parity trials against torch.topk pass across row counts 37-862, widths 4097-51720, and fp16-overflow/exact-tie distributions. Kernel microbench on (862, 51720) k=2048: 0.115 -> 0.129 ms/call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe varlen indexer top-k kernel now excludes out-of-bounds lanes from histogram updates, threshold processing, and index writes. A parametrized SM90+ regression test verifies valid indices and agreement with ChangesOOB tile lane handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/python/fe_api/dsa/test_DSA_indexer_top_k.py`:
- Around line 209-214: In the row-validation assertions around the DSA top-k
result, add a check that the valid entries in indices[r] are unique before
comparing values with torch.topk. Use the existing valid mask and preserve the
current count, range, and value assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 96e8c3db-c70d-4bec-9fd4-c95d2024b54e
📒 Files selected for processing (2)
python/cudnn/deepseek_sparse_attention/indexer_top_k/indexer_top_k_varlen_util.pytest/python/fe_api/dsa/test_DSA_indexer_top_k.py
| valid = indices[r] >= 0 | ||
| assert int(valid.sum()) == k, f"row {r}: expected {k} valid indices, got {int(valid.sum())}" | ||
| assert int(indices[r].max()) < L, f"row {r}: out-of-range index {int(indices[r].max())} >= seq_len {L}" | ||
| got = values[r][valid].sort(descending=True).values | ||
| ref = torch.topk(scores[r, :L], k).values.sort(descending=True).values | ||
| torch.testing.assert_close(got, ref, atol=1e-6, rtol=1e-5) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert selected indices are unique.
For trigger_dist="identical", duplicate in-range indices can still produce the same sorted values as torch.topk, masking a broken selection result.
Suggested assertion
valid = indices[r] >= 0
assert int(valid.sum()) == k, f"row {r}: expected {k} valid indices, got {int(valid.sum())}"
- assert int(indices[r].max()) < L, f"row {r}: out-of-range index {int(indices[r].max())} >= seq_len {L}"
+ selected_indices = indices[r][valid]
+ assert int(selected_indices.max()) < L, f"row {r}: out-of-range index {int(selected_indices.max())} >= seq_len {L}"
+ assert selected_indices.unique().numel() == k, f"row {r}: duplicate selected indices"
got = values[r][valid].sort(descending=True).values📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| valid = indices[r] >= 0 | |
| assert int(valid.sum()) == k, f"row {r}: expected {k} valid indices, got {int(valid.sum())}" | |
| assert int(indices[r].max()) < L, f"row {r}: out-of-range index {int(indices[r].max())} >= seq_len {L}" | |
| got = values[r][valid].sort(descending=True).values | |
| ref = torch.topk(scores[r, :L], k).values.sort(descending=True).values | |
| torch.testing.assert_close(got, ref, atol=1e-6, rtol=1e-5) | |
| valid = indices[r] >= 0 | |
| assert int(valid.sum()) == k, f"row {r}: expected {k} valid indices, got {int(valid.sum())}" | |
| selected_indices = indices[r][valid] | |
| assert int(selected_indices.max()) < L, f"row {r}: out-of-range index {int(selected_indices.max())} >= seq_len {L}" | |
| assert selected_indices.unique().numel() == k, f"row {r}: duplicate selected indices" | |
| got = values[r][valid].sort(descending=True).values | |
| ref = torch.topk(scores[r, :L], k).values.sort(descending=True).values | |
| torch.testing.assert_close(got, ref, atol=1e-6, rtol=1e-5) |
🤖 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_indexer_top_k.py` around lines 209 - 214, In
the row-validation assertions around the DSA top-k result, add a check that the
valid entries in indices[r] are unique before comparing values with torch.topk.
Use the existing valid mask and preserve the current count, range, and value
assertions.
|
Closing as a duplicate of #410, which predates this PR and carries an essentially identical fix (same OOB-lane guards in the same three loops). Production evidence and an additional trigger route (fp16-collapse of scores below −65504 into the |
Before submitting
black --line-length 160on the changed files.Fixes #446
Affected area
FE OSS kernels or CuTeDSL (DSA
indexer_top_k).Summary
The CuTe-DSL radix top-k kernel reads each row in fixed-width vector tiles. The final partial tile's out-of-bounds lanes are filled with
-infby_fill_oobso the predicated copy is memory-safe — but the histogram and candidate-collection loops then iterate every fragment lane without a bounds check, counting those phantom-inflanes as real elements.This is harmless while a row's top-k threshold sits above the phantom bin, because phantom
-inflanes sort last and are dropped. However,to_coarse_keycomputes the coarse radix bin from the fp16 conversion of the fp32 score, so every score below fp16's −65504 minimum collapses into the fp16-infbin — the same bin the phantom lanes occupy. When a row's top-k threshold lands inside that bin (fewer thantop_kvalues above ~−65504), the threshold bin's candidate list becomesreal_count + up-to-tile-width phantom lanes:large_occupancycompile (num_rows > 148), the per-row candidate buffers (512-entry smem +num_colsgmem) overflow → out-of-bounds shared/global writes →cudaErrorIllegalAddress;Observed in production
A GLM-5.2 context-parallel (CP32) training run on B200s crashed with Xid 43 (
illegal memory access) mid-forward_backward— once in 5 hours, sequence-length-independent, no ECC errors. The crash is deterministically data-dependent: the captured score tensor reproduces it in a fresh process, on both B200 and B300, at the same call for the same data.CUDA_LAUNCH_BLOCKING=1attributes the fault to this kernel;compute-sanitizerreports invalid 2-byte shared writes; bisection isolated the trigger to a single row (seq_len 4122 of width 4310, 2242 values below −65504) plus the >148-row compile.Fix
Skip lanes whose tile column coordinate is out of bounds (
col < aligned_size) in the three vectorized histogram/collection loops — the same bound the predicated copy already computes viapredicate_tile. The scalar prologue/leftover loops are exact-bounded and unchanged. Selection semantics for in-bounds elements are untouched.Testing
test_DSA_indexer_top_k_oob_tile_lanesdeterministically drives a row into the flood regime (149 rows →large_occupancy; 4310-wide; a 4122-long row with <top_kvalues above the fp16 negative-overflow point), in two variants: identical collapsed values (fails pre-patch with out-of-range indices) and distinct collapsed values (pre-patch:cudaErrorIllegalAddress). Both pass post-patch.develop) on B200 and B300: previously-crashing inputs complete with in-range indices,compute-sanitizerreports no invalid accesses, and 12/12 randomized parity trials againsttorch.topkpass (rows 37–862, widths 4097–51720, fp16-overflow and exact-tie distributions, both kernel compiles).Related
top_kstore width) — same kernel, disjoint code (that fix is the phase-3 output store; this one is phases 1–2 selection).