Introduce fused GEMM + 1-NN primitive using cuTile - #2249
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
There was a problem hiding this comment.
Many thanks @divyegala for the comprehensive PR!
I double-verified the functional correctness of the kernel from this PR. The kernel also matches one-to-one the performance with the one from gitlab.
dantegd
left a comment
There was a problem hiding this comment.
Could we make the path selection and workspace allocation agree on whether the cuTile launch is guaranteed to succeed? When minClusterDistanceCompute.cu selects FusedCutile, it doesn’t allocate the CUTLASS KVP scratch or mutex workspace. But if try_fused_1nn_tile() returns false, cpp/src/distance/detail/fused_distance_nn.cuh falls through to CUTLASS and expects those buffers to exist. That leaves us without a valid fallback if launcher construction fails, even though the try_ API suggests falling back is supported. Would it make sense to build or probe the launcher before committing to FusedCutile, or keep the fallback workspace allocated for as long as this path can still return false?
| cutlass_kvp_scratch, | ||
| stream); | ||
| break; | ||
| case cuvs::distance::DistanceType::InnerProduct: break; |
There was a problem hiding this comment.
Could we route InnerProduct to a real non-cuTile implementation when cuTile isn't built or available?
On CUDA 12, try_fused_1nn_tile is compiled as false, so this function initializes the scratch output to (0, max), hits this empty case, and then unpacks that untouched sentinel. The new input_fp32_fused test includes InnerProduct unconditionally, so the supported CUDA 12 configuration returns incorrect results. What do you think about selecting the unfused path upstream for InnerProduct when cuTile is unavailable, or implementing the fallback here?
There was a problem hiding this comment.
The unfused path does not support InnerProduct for some reason. Let me see if we can support or just throw for now.
| static_assert(std::is_same_v<IdxT, int> || std::is_same_v<IdxT, int64_t>); | ||
|
|
||
| constexpr int strict_pitch_elements = 16 / sizeof(DataT); | ||
| const bool use_strict_abi = k % strict_pitch_elements == 0; |
There was a problem hiding this comment.
Both the strict and relaxed signatures declare base_addr_divisible_by=16 for matrices, norms, indices, and distances, but this selection only checks row pitch. Valid RAFT subviews or DLPack byte offsets can therefore launch under a false compiler assumption; the int64 loop below also makes xn + max_i32 and nearest_dist + max_i32 non-16-byte-aligned on its second batch. cuTile documents violating an exported signature assumption as undefined behavior: “Launching the exported kernel with an array that doesn’t satisfy this assumption would then result in undefined behavior.”
Should we validate base alignment for every non-null array before launching, or export a base-alignment-1 fallback? What do you think about making alignment part of eligibility before choosing either ABI?
There was a problem hiding this comment.
Oh I did not think at all about subviews or offsets, good catch. For now, let's just validate base pointer alignment. I can add to the follow-up issue to validate 1-byte alignment and what kind of cubin is produced by cutile?
| case cuvs::distance::DistanceType::InnerProduct: { | ||
| // TODO: pass buffer | ||
| rmm::device_uvector<MathT> distances(n_rows * n_clusters, stream, mr); | ||
| if (uses_fused_distance_nn( |
There was a problem hiding this comment.
Could we make this decision using the same pointer-aware eligibility check as min_cluster_and_distance()? On CUDA 13, use_fused() can enter this branch before checking the actual X, centroid, and labels pointers. If any of those rejects cuTile, min_cluster_and_distance() falls back through use_legacy_fused(); for InnerProduct that means Unfused, which eventually calls pairwise_distance_kmeans() and throws because it only supports L2 metrics. CUDA 12 instead reaches the GEMM+argmin path below. What do you think about resolving final cuTile eligibility here and taking this existing GEMM fallback whenever it isn’t eligible?
|
|
||
| switch (fused_path) { | ||
| case FusedDistancePath::FusedCutile: | ||
| // Conservatively budget the fallback in case the eventual pointer-aware probe fails. |
There was a problem hiding this comment.
Could we budget the actual architecture-specific fallback here? On Blackwell—and on Hopper when both the row and cluster counts are below the legacy fused threshold—a pointer-aware cuTile rejection selects Unfused. That path allocates a batch-by-cluster distance matrix plus KVP output, but this case only charges int + KVP per row. Because predict_core() passes zero batch limits, the next allocation can exceed the 512 MiB target by a large factor. What do you think about budgeting the maximum of the cuTile scratch and the use_legacy_fused() scratch rather than assuming every fallback is CUTLASS?
| { | ||
| if (!runtime_cuda13_or_newer()) { return false; } | ||
| if (has_embedded_cubin_for_arch(cc_major, cc_minor)) { return true; } | ||
| return tileir_fallback_available(driver_version); |
There was a problem hiding this comment.
I wonder if we shoukd gate the TileIR fallback on cuTile-supported compute capabilities? RAPIDS CUDA 13 still supports Turing/SM75, while cuTile requires compute capability 8.x through 12.x. On SM75 with a new enough driver, this returns true, so KMeans selects FusedCutile and attempts to load TileIR instead of keeping the supported legacy CUDA path. What do you think about checking the supported architecture range before reporting availability?
| { | ||
| cudaLibrary_t library{}; | ||
| RAFT_CUDA_TRY( | ||
| cudaLibraryLoadData(&library, image.data, nullptr, nullptr, 0, nullptr, nullptr, 0)); |
There was a problem hiding this comment.
Should we preserve the nullable try_get_launcher() contract for expected module compatibility and JIT failures?
If I'm not mistaken, RAFT_CUDA_TRY here turns errors such as cudaErrorNoKernelImageForDevice, cudaErrorUnsupportedPtxVersion, and cudaErrorJitCompilerNotFound into exceptions, so can_launch_fused_1nn_tile() never returns false and KMeans cannot take its legacy fallback.
What do you think about treating expected compatibility/JIT errors as a failed build, unloading the library if kernel lookup fails, while still propagating genuine resource failures such as allocation errors?
| raft::make_const_mdspan(minClusterAndDistance.view()), | ||
| raft::make_device_vector_view<LabelT, IdxT>(labels, n_rows), | ||
| raft::compose_op<raft::cast_op<LabelT>, raft::key_op>()); | ||
| auto nearest_dist = |
There was a problem hiding this comment.
Could we keep native KVP storage when cuTile isn’t selected? On CUDA 12 this always allocates nearest_dist—and nearest_idx when LabelT differs, then minClusterAndDistanceCompute() allocates another full KVP because it was given SoA outputs.
The old path used the KVP output directly, so this adds avoidable per row scratch to every legacy L2/cosine balanced prediction; the hidden KVP also uses the current RMM resource rather than mr.
What do you think about deciding the path first and using minClusterAndDistanceComputeKvp() for legacy paths, as process_batch() does?
| weight_per_cluster, | ||
| batch_workspace, | ||
| /*reset_sums=*/false); | ||
| auto weighted_dist = raft::make_device_vector<DataT, IndexT>(handle, n_samples); |
There was a problem hiding this comment.
We can we avoid materializing weighted_dist on the cuTile path, no?
The KVP branch just below feeds a transform iterator over distance × weight into computeClusterCostFromIterator(), and the same approach works with nearest_dist here. As written, every cuTile Lloyd batch carries an extra n_samples * sizeof(DataT) allocation. What do you think about using the same iterator pattern in both branches?
| ndim=2, | ||
| index_dtype=index_dtype, | ||
| stride_lower_bound_incl=(0, None), | ||
| alias_groups=(), |
There was a problem hiding this comment.
Question: are callers allowed to use overlapping read-only dataset and centroid views? I couldn’t find a non-aliasing precondition on the KMeans APIs, while an empty alias_groups sequence tells cuTile that this array cannot alias any other parameter. If that overlap is supported, passing the same storage for X and centroids invokes the exported kernel outside its declared signature. Would common alias groups for the read only inputs or an explicit eligibility check and documented precondition make the contract clearer?v
There was a problem hiding this comment.
They are not, but when is this a realistic path? This is akin to adding __restrict__ on each input pointer, right? When would centroids and X be the exact same pointer?
# Conflicts: # python/libcuvs/pyproject.toml
This PR adds infrastructure built on top of existing JIT LTO architecture to generate kernels using
cutile-pythonat build time, and embed them in the C++ library to make them callable from C++.Merge #2548 first.