Pipeline direct NVLink dispatch with triple-buffered prefetch - #693
Pipeline direct NVLink dispatch with triple-buffered prefetch#693usernamehaha2022 wants to merge 3 commits into
Conversation
| constexpr int kNumTmaBuffers = kIsScaleupNVLink ? 3 : 1; | ||
| const auto warp_tma_buffers = layout::BufferLayout<true>(token_layout, kNumDispatchWarps, kNumTmaBuffers, | ||
| math::advance_ptr<int>(smem, kNumSmemBytesForNotify)).get_rank_buffer(dispatch_warp_idx); | ||
| auto recv_buffer = layout::BufferLayout<false>(token_layout, kNumRanks, kNumMaxTokensPerRank, buffer); | ||
| auto send_buffer = layout::BufferLayout<false>(token_layout, 1, kNumMaxTokensPerRank, recv_buffer.get_buffer_end_ptr()); | ||
| recv_buffer = recv_buffer.get_rank_buffer(rank_idx); | ||
|
|
||
| // Init TMA | ||
| ptx::arrival_phase phase = 0; | ||
| const auto mbarrier_ptr = tma_buffer.get_mbarrier_ptr(); | ||
| if (ptx::elect_one_sync()) | ||
| ptx::mbarrier_init_with_fence(mbarrier_ptr, 1); | ||
| ptx::arrival_phase phases[kNumTmaBuffers] = {}; | ||
| #pragma unroll | ||
| for (int i = 0; i < kNumTmaBuffers; ++ i) { | ||
| const auto tma_buffer = warp_tma_buffers.get_token_buffer(i); | ||
| if (ptx::elect_one_sync()) | ||
| ptx::mbarrier_init_with_fence(tma_buffer.get_mbarrier_ptr(), 1); |
There was a problem hiding this comment.
🟡 warning: The per-warp buffer count is hardcoded in the kernel (kIsScaleupNVLink ? 3 : 1), while the host sizes smem and caps warps using a separate kNumDirectNVLinkTmaBuffers/num_tma_buffers. The reuse-safety of round-robin prefetch (overwriting a staging buffer whose consuming remote store may still be in flight) relies on a non-local invariant: the count must exceed the number of distinct store groups that remain after tma_store_wait<1()> (here 1 remaining younger group, on a different buffer). This breaks if the two sources of truth diverge or the count changes. Template the count into the kernel and static-assert the invariant (or at least kNumTmaBuffers > 2).
🤖 v4
| // to the group that this wait just retired. | ||
| ptx::tma_store_wait<1>(); | ||
| __syncwarp(); | ||
| prepare_token(next_token_idx, next_token_iter, next_dst_rank_idx, next_dst_slot_idx); | ||
| } |
There was a problem hiding this comment.
🔵 suggestion: The comment claims the next-token buffer is the group 'just retired' by the tma_store_wait<1>(). That is only true in steady state for exactly 3 buffers with the youngest group left behind; the exact token_iter -> buffer -> store-group mapping is non-obvious and easy to break (e.g. if the NVLink and RDMA paths use different counts). Document/assert it, or use a single source of truth for the count.
🤖 v4
| auto& phase = phases[tma_buffer_idx]; | ||
| // Wait TMA load arrival | ||
| // NOTES: this arrive must be after the `ptx::cp_async_mbarrier_arrive` | ||
| if (ptx::elect_one_sync()) { |
There was a problem hiding this comment.
🔵 suggestion: The triple-buffer safety relies on the invariant that at most 2 store groups are outstanding when tma_store_wait<1>() runs, so the buffer being recycled ((N+1)%3 == (N-2)%3) is exactly the one whose store this wait retires. This coupling between kNumTmaBuffers==3 and the wait depth <1> is implicit. Consider adding a brief comment documenting the 3-buffers/1-outstanding invariant, since if the buffer count is ever changed the wait depth must change in lockstep.
🤖 v3
|
|
||
| constexpr int kNumNotifyWarps = 4; | ||
| constexpr int kNumDirectNVLinkDispatchWarps = 14; | ||
| constexpr int kNumDirectNVLinkTmaBuffers = 3; |
There was a problem hiding this comment.
🔵 suggestion: kNumDirectNVLinkTmaBuffers=3 (host, used for shared-memory sizing) must stay in sync with the device-side constexpr int kNumTmaBuffers = kIsScaleupNVLink ? 3 : 1; in dispatch.cuh. These two constants are defined independently in different files; a mismatch would silently under/over-allocate shared memory relative to what the kernel indexes. Consider unifying them into a single shared constant to prevent future drift.
🤖 v3
🤖 ds-review-bot Code Reviewv6The multi-buffer TMA scheduling and matching shared-memory sizing appear consistent across NVLink and non-NVLink paths, with outstanding stores flushed before buffer reuse. v4The dispatch kernel switches from single per-warp staging (drained fully per token) to three round-robin staging TMA buffers per dispatch warp. While the current token's remote S2G/NVLink/RDMA store stays in flight, the next token's hidden/SF/metadata/routing is prefetched into the buffer whose previous use is the oldest outstanding store group (deterministically token_iter - 2 of 3, or none on a warm-up iteration); tma_store_wait<1>() retires that group before the buffer is overwritten, and the final barrier drains the last in-flight store. The host sizes per-warp smem by 3x (RDMA: 1x) and caps NVLink dispatch warps at 14, with the RDMA path remaining single-buffered and full-drained (unchanged behavior). Edge cases (first iteration, last token skipped, warps with no tokens, per-token global slot/counter writes) are all safe and unchanged. Measured on 8xH100 dispatch goes 250->280 GB/s, combine unchanged. Overall the change correctly and faithfully implements the PR as a pure pipelining/perf optimization on the previously single-buffered path. v3This PR pipelines direct NVLink dispatch by switching from double- to triple-buffered TMA staging (kNumDirectNVLinkTmaBuffers 2->3, kNumTmaBuffers 2->3 for the NVLink path), and refactoring the per-token body into a Correctness analysis (all verified sound):
Overall the change is internally consistent and the pipelining logic is sound. The two comments below are minor maintainability suggestions, not correctness issues. Files reviewed: 2 |


Summary
This PR pipelines direct NVLink dispatch using three shared-memory TMA buffers per dispatch warp.
Previously, a dispatch warp prepared tokens sequentially around remote S2G stores. With this change, the next token's hidden data, scale factors, metadata, and routing information are prefetched while previous remote stores remain in flight.
At steady state, the three buffers are used for:
Before recycling the oldest buffer,
tma_store_wait<1>()reduces the number of outstanding store groups to one. This keeps buffer reuse safe without fully draining all remote stores between tokens.The shared-memory calculation now accounts for all three staging buffers, and direct NVLink dispatch is limited to at most 14 dispatch warps. The non-NVLink path remains single-buffered.
Validation
Tested on 8 x NVIDIA H100 GPUs with direct NVLink dispatch.
Average across 8 ranks:
mainWith rank unbalanced ratio set to 1.5:
main