Skip to content

[DSA] Fix top-k v2 dropping non-primary ranks' output on CUDA 13.1+ (root cause for #33835) - #34167

Merged
BBuf merged 2 commits into
sgl-project:mainfrom
DarkSharpness:dsv4-topk-cuda13-dsmem-fix
Aug 10, 2026
Merged

BBuf merged 2 commits into
sgl-project:mainfrom
DarkSharpness:dsv4-topk-cuda13-dsmem-fix

Conversation

@DarkSharpness

@DarkSharpness DarkSharpness commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Motivation

DeepSeek-V4 DSA decode can return a corrupt top-k from the fused small-batch
cluster kernel, which downstream sparse attention then dereferences as garbage KV
indices (the illegal memory access reported in #33835).

The trigger is the CUDA toolkit version, not the GPU architecture: identical
source and identical hardware (H200, sm_90a) are correct when built with nvcc
12.9/13.0 and wrong when built with 13.1/13.2/13.3.

Root cause

TopKCluster::forward picked its phase-3 scatter destination up front:

const auto cur_out = is_primary ? problem.out : smem->tmp_out;
...
cur_out[pos] = idx;

problem.out can be a shared::cluster (DSMEM) alias of the elected rank's
buffer, while smem->tmp_out is shared::cta. Merging both into one pointer
variable makes cicc 13.1+ mis-lower the block-local arm on sm_90a and silently
drop every non-primary rank's staged output
tmp_out stays zero, and phase
3.5 then faithfully copies zeros to perfectly correct DSMEM addresses.

Slot-diffing one row against a CUDA 12.9 build shows exactly one contiguous bad
run:

correct slots: 129/512      bad-slot runs: [(61, 443, 383)]

Slots 0–60 are the primary's own scatter and 444–511 are the handle_tie tail;
the 383 lost slots are exactly the seven peers' contributions. Read-back
instrumentation confirms the mechanism — tmp_out[0] == 0 on all seven peers
under 13.3 versus valid indices (8161, 10064, 16264, …) under 12.9, while
count_gt reports 34–64 candidates found.

Verified not the cause (each tested and ruled out): enable_smem_spilling,
__restrict__ on TopKProblem::out, the store's address space (forcing a
generic st.u32 at the emit changes nothing), the load side (volatile loads),
and memory ordering (explicit barrier.cluster.arrive.release /
wait.acquire, __threadfence()). Pointer values, start_write/num_write,
and the store addresses were all dumped and are correct in both toolchains
only the data was wrong.

Changes

1. Keep the two scatter destinations in separate code paths so neither
pointer ever carries two address spaces, with a comment so nobody merges them
back. This is the actual fix.

2. Drop the peer_problem copy from #32910, stating the block-local pointer
at the read-back site instead. That __builtin_assume is load-bearing, not an
optimization: removing it reproduces the #32830 cicc segfault on 13.1/13.2/13.3.

3. Make top-k v1 take a runtime topk (second commit, separable). v1 baked
topk in via -DSGL_TOPK, building one module per k; since the macro fed a
constexpr rather than a template parameter, the modules exported identically
mangled symbols, and setup_kernel_smem_once's function-local static is emitted
as STB_GNU_UNIQUE — merged across every loaded object regardless of
RTLD_LOCAL. The module used second therefore skipped its
cudaFuncSetAttribute opt-in and failed to launch with 64 KB of dynamic shared
memory (CUDA error: invalid argument). bench_topk.py hits this because it
sweeps k in {512, 1024} in one process; a server only ever uses one k.

Scope correction vs #33835

#33835 attributes this to Hopper and to rows just above the 32K small-batch
floor. Measured here, every fused small-batch cluster shape is affected
(batch <= 30 and seq_len > cluster_floor), including 65537 and 98304 which
that PR reports as fine — the floor only makes the path reachable at
batch <= 15. The persistent-pool path was never affected because it stages
output in global memory. With the root cause fixed, no arch gate is needed and
the fused path keeps working on Hopper.

The boundary regression test added by #33835 is still worth keeping; it covers a
real gap (batch <= 15 with seq_len in (32768, 65536]).

Validation

H200 (sm_90a), 157-row suite = the report's shapes + a boundary sweep +
register/streaming/persistent controls, k in {512, 1024, 2048}:

nvcc before after
12.9 0 bad 0 bad
13.0 0 bad 0 bad
13.1 157 bad 0 bad
13.2 157 bad 0 bad
13.3 157 bad 0 bad

Those are all five stable nvcc minors >= 12.9. Additionally: 1500-iteration
randomized stress and 500 CUDA-graph replays clean; builds for sm_90a and
sm_100a
on every toolchain; v1's runtime-topk path passes 80 cases spanning
k in {1, 2, 7, 31, 64, 100, 255, 256, 257, 300, 511, 512, 513, 777, 1023, 1024}
against trivial / boundary / radix shapes.

No performance regression (CUDA 12.9, where both old and new are correct, so
the comparison is at equal correctness):

case main this PR
register 8x8192 3.882 3.884
streaming 100x65536 14.276 14.261
fused-cluster 8x33000 8.387 8.230
fused-cluster 8x98304 9.431 9.309
fused-cluster 30x131072 13.805 13.564
1 long + 7 short (report shape) 9.740 9.575
persistent 128x131072 42.010 42.025

Worst case +0.1% (noise); the fused cluster path gets 1.3–2.9% faster.

Caveats

  • sm_100a is compile-verified only — no Blackwell was available to run on.
    Runtime confirmation there would be welcome.
  • CI builds cu130, which is a clean cell, so CI cannot currently catch this
    class of bug
    . Worth considering a toolkit-version guard or moving a build to
    13.1+.
  • The remaining exported-weak-symbol hazard in v1 (the kernel host stub itself)
    is not addressed here. Compiling JIT modules with hidden visibility would be
    the general fix; it matters because that half fails silently rather than
    erroring.

Checklist

  • Root cause identified and the disproven hypotheses documented
  • Verified across every stable nvcc minor >= 12.9, before and after
  • Performance measured at equal correctness; no regression
  • Blackwell (sm_100a) runtime validation
  • CI

🤖 Generated with Claude Code


CI States

Latest PR Test (Base): ❌ Run #31310567974
Latest PR Test (Extra): ❌ Run #31310567856

@DarkSharpness
DarkSharpness force-pushed the dsv4-topk-cuda13-dsmem-fix branch from 2b608d1 to 0718af7 Compare August 9, 2026 10:40
@DarkSharpness DarkSharpness added bypass-fastfail run-ci CI: run the baseline test suite on this PR labels Aug 9, 2026
…3.1+

`TopKCluster::forward` selected its phase-3 scatter destination up front:

    const auto cur_out = is_primary ? problem.out : smem->tmp_out;

`problem.out` can be a `shared::cluster` (DSMEM) alias of the elected rank's
buffer, while `tmp_out` is `shared::cta`. Merging both into one pointer variable
makes cicc 13.1+ mis-lower the block-local arm for sm_90a and silently drop
every non-primary rank's staged output: `tmp_out` stays zero, and phase 3.5 then
faithfully copies zeros to perfectly correct DSMEM addresses.

The result is a top-k row where only the primary's slots and the `handle_tie`
tail hold valid indices. Slot-diffing one row against a CUDA 12.9 build shows
exactly one contiguous bad run -- [61, 443] -- with slots 0-60 (the primary's own
scatter) and 444-511 (handle_tie) correct. Downstream sparse attention then
dereferences the garbage slots, which is the illegal memory access reported in
sgl-project#33835.

Fix: keep the two destinations in separate code paths so neither pointer ever
carries two address spaces, and note it so nobody merges them back.

Scope: affects any fused small-batch cluster shape (`batch <= 30` and
`seq_len > cluster_floor`), not just rows near the 32K small-batch floor -- the
floor only makes the path reachable at `batch <= 15`. The persistent-pool path
was never affected because it stages output in global memory.

Toolchain matrix (H200, sm_90a, 157-row suite over the report's shapes plus a
boundary sweep and register/streaming/persistent controls):

    nvcc     before        after
    12.9     0 bad         0 bad
    13.0     0 bad         0 bad
    13.1     157 bad       0 bad
    13.2     157 bad       0 bad
    13.3     157 bad       0 bad

CI builds cu130, which is a clean cell -- that is why this stayed hidden and why
the report came from a CUDA 13.1 deployment.

Also drops the `peer_problem` copy from sgl-project#32910 and states the
block-local pointer at the read-back site instead. That `__builtin_assume` is
load-bearing: removing it reproduces the sgl-project#32830 cicc segfault on 13.1/13.2/13.3.

Validation: 0 bad rows on 12.9/13.0/13.1/13.2/13.3; 1500-iteration randomized
stress and 500 CUDA-graph replays clean; builds for sm_90a and sm_100a on every
toolchain. No performance regression -- worst case +0.1% (noise), and the fused
cluster shapes get 1.3-2.9% faster. sm_100a is compile-verified only; no
Blackwell was available to run on.
@DarkSharpness
DarkSharpness force-pushed the dsv4-topk-cuda13-dsmem-fix branch from 0718af7 to 9fa75d5 Compare August 9, 2026 11:10
v1 baked `topk` in via `-DSGL_TOPK`, so `topk_transform_512` built a separate
module per k (512 / 1024). Because the macro fed a `constexpr` rather than a
template parameter, both modules exported identically mangled symbols -- and
`setup_kernel_smem_once`'s function-local static is emitted as STB_GNU_UNIQUE,
which the loader merges across every loaded object regardless of RTLD_LOCAL.

So whichever module was used *second* saw the static already initialized, skipped
its `cudaFuncSetAttribute` opt-in, and then tried to launch with 64 KB of dynamic
shared memory against the 48 KB default:

    RuntimeError: ... topk_v1.cuh:336: CUDA error: invalid argument

Symmetric in k -- whichever k ran first won. `test/registered/kernels/benchmark/
attention/bench_topk.py` hits it because it sweeps k in {512, 1024} in one
process; a server only ever uses one k, which is why production never saw it.

Making `topk` a runtime parameter leaves one module, so the collision cannot
happen by construction. The block size stays fixed at 1024 rather than tracking
`topk`: `run_cumsum()` and the histogram init index up to `RADIX + 1 == 257`
threads, so sizing the block after a small topk would silently skip part of the
histogram.

The remaining exported-weak-symbol hazard (the kernel host stub itself) is not
addressed here; compiling JIT modules with hidden visibility would be the general
fix, and matters because that half fails *silently* rather than erroring.

Validation: 80 cases pass -- k in {1, 2, 7, 31, 64, 100, 255, 256, 257, 300, 511,
512, 513, 777, 1023, 1024} x {trivial path, seq == topk, radix path, 8x4096,
2x65536}, checking slot coverage, index range, agreement with torch.topk modulo
tie swaps, and raw_indices consistency. k > 1024 now fails cleanly. Latency is
unchanged for k=1024 and improves substantially for long context (1x65536:
67.6 -> 41.1 us) since the doubled block size doubles load parallelism in the
strided passes; batch=1024 x seq=4096 regresses ~14%, which is acceptable as v1
is being superseded by v2.
@DarkSharpness
DarkSharpness force-pushed the dsv4-topk-cuda13-dsmem-fix branch from 9fa75d5 to d7e6ea4 Compare August 9, 2026 11:21

@BBuf BBuf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It‘s a UB fix, LGTM.

@BBuf

BBuf commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

@BBuf
BBuf merged commit accc51c into sgl-project:main Aug 10, 2026
135 of 179 checks passed
Leoyzen pushed a commit to Leoyzen/sglang that referenced this pull request Aug 14, 2026
…root cause for sgl-project#33835) (sgl-project#34167)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
saturn-acc pushed a commit to saturn-acc/sglang that referenced this pull request Aug 16, 2026
…root cause for sgl-project#33835) (sgl-project#34167)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Atituiset pushed a commit to Atituiset/sglang that referenced this pull request Sep 10, 2026
…root cause for sgl-project#33835) (sgl-project#34167)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bypass-fastfail jit-kernel run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants