Repository navigation
[PyTorch]Validate grouped split alignment on device - #3745
Conversation
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
|
/te-ci L0 |
[Medium impact] The PR appears safe to merge, with no new actionable issues found. SummaryAdds device-side split alignment checks to the existing offset scan. The original C API keeps its behavior, while PyTorch callers can request alignment.
Reviews (6) · Last reviewed commit: "Apply suggestion from @timmoon10" · Reviewed by Greptile |
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
timmoon10
left a comment
There was a problem hiding this comment.
Overall LGTM, with minor suggestions.
| // Validate before the scan. All threads leave together so no thread waits at a barrier | ||
| // after another thread has trapped on invalid split metadata. | ||
| const int64_t split = | ||
| idx < args.num_splits ? load_split_size(args.split_sizes, args.split_sizes_dtype, idx) : 0; | ||
| if (args.split_alignment > 1) { | ||
| const size_t alignment_mask = args.split_alignment - 1; | ||
| bool misaligned; | ||
| if ((args.split_alignment & alignment_mask) == 0) { | ||
| misaligned = (static_cast<size_t>(split) & alignment_mask) != 0; | ||
| } else { | ||
| misaligned = static_cast<size_t>(split) % args.split_alignment != 0; | ||
| } | ||
| const bool invalid = idx < args.num_splits && (split < 0 || misaligned); | ||
| if (__syncthreads_or(invalid)) { | ||
| if (tid == 0) { | ||
| NVTE_DEVICE_ERROR("Grouped split size is not divisible by split_alignment."); | ||
| __trap(); // NVTE_DEVICE_ERROR's assert is compiled out of release builds. | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| block_scan[tid] = split; | ||
| __syncthreads(); |
There was a problem hiding this comment.
This is fine, but bitmasking seems overkill for a kernel that's unlikely to be compute-bound. The following is a little simpler.
| // Validate before the scan. All threads leave together so no thread waits at a barrier | |
| // after another thread has trapped on invalid split metadata. | |
| const int64_t split = | |
| idx < args.num_splits ? load_split_size(args.split_sizes, args.split_sizes_dtype, idx) : 0; | |
| if (args.split_alignment > 1) { | |
| const size_t alignment_mask = args.split_alignment - 1; | |
| bool misaligned; | |
| if ((args.split_alignment & alignment_mask) == 0) { | |
| misaligned = (static_cast<size_t>(split) & alignment_mask) != 0; | |
| } else { | |
| misaligned = static_cast<size_t>(split) % args.split_alignment != 0; | |
| } | |
| const bool invalid = idx < args.num_splits && (split < 0 || misaligned); | |
| if (__syncthreads_or(invalid)) { | |
| if (tid == 0) { | |
| NVTE_DEVICE_ERROR("Grouped split size is not divisible by split_alignment."); | |
| __trap(); // NVTE_DEVICE_ERROR's assert is compiled out of release builds. | |
| } | |
| return; | |
| } | |
| } | |
| block_scan[tid] = split; | |
| __syncthreads(); | |
| // Load input from global memory into shared memory | |
| if (idx < args.num_splits) { | |
| block_scan[tid] = load_split_size(args.split_sizes, args.split_sizes_dtype, idx); | |
| } else { | |
| block_scan[tid] = 0; | |
| } | |
| if (args.split_alignment > 1) { | |
| // Check that splits have required alignment | |
| if (__syncthreads_or(block_scan[tid] % args.split_alignment)) { | |
| if (tid == 0) { | |
| NVTE_DEVICE_ERROR("Grouped split size is not divisible by split_alignment."); | |
| __trap(); // NVTE_DEVICE_ERROR's assert is compiled out of release builds. | |
| } | |
| return; | |
| } | |
| } else { | |
| __syncthreads(); | |
| } |
There was a problem hiding this comment.
I evaluated this change. And not doing the bit masking costs us 0.07us consistently for the kernel.
| if (num_splits <= s2o::kSmallThreadsPerBlock) { | ||
| s2o::kernel<s2o::kSmallThreadsPerBlock><<<1, s2o::kSmallThreadsPerBlock, 0, stream>>>(args); | ||
| } else { | ||
| s2o::kernel<s2o::kThreadsPerBlock><<<1, s2o::kThreadsPerBlock, 0, stream>>>(args); | ||
| } |
There was a problem hiding this comment.
Is there reason to support separate kernels for these two cases? This kernel seems launch-bound, so the smaller variant doesn't give us much perf benefit.
There was a problem hiding this comment.
The improves perf by about 0.2 us. I would assume this is purely warp scheduling overhead (4 vs 8 warps). But at the scale of 2-3 us for the kernel that is 10% perf benefit. And so I kept this change. But you can see that we are loosing the perf by about 0.2 us due to the introduction of alginment check.
Co-authored-by: Tim Moon <4406448+timmoon10@users.noreply.github.com> Signed-off-by: vthumbe1503 <vthumbe@nvidia.com>
Co-authored-by: Tim Moon <4406448+timmoon10@users.noreply.github.com> Signed-off-by: vthumbe1503 <vthumbe@nvidia.com>
Description
Validate grouped split sizes inside the existing device-side multi-output offset scan. GroupedLinear requests 128-row alignment; the generic grouped MLP offset path requests 256-row alignment. The check runs on every CUDA graph replay without a host read or an added kernel launch.
nvte_splits_to_offsets_multi_v2(split_sizes, split_alignment, outputs, ...)exposes the check. The original C API symbol remains and forwards with alignment 1. The PyTorch private binding accepts an optionalsplit_alignment.Type of change
Changes
NVTE_DEVICE_ERRORprints and asserts;__trap()keeps the failure effective in Release builds whereNDEBUGremoves the assert.NVTE_BUILD_DEBUG=1enables the assert when building, not at runtime.Offset-scan GPU time on GB200
Measured the one- and four-output scan with 8 and 16 experts, 256 rows per expert, and
split_alignment=128on the PR. The four-output call uses strides[1,1,7168,4096], matching a grouped linear FC1 shape. Inputs are CUDA int64 splits; the first output is int32 and the remaining outputs are int64. Four-output calls use bulk allocation.GPU times are medians of seven samples, each timing 40 replays of a CUDA graph containing 64 scan calls. Baseline
e32e1238and PR06228625used the same GB200 and container, with their respective built extensions.Checklist