Vamana build optimization set and fp16 support - #2264
Conversation
(cherry picked from commit 14e36b3)
(cherry picked from commit d8b547b)
(cherry picked from commit 4d970e0)
…L2 comparators. (cherry picked from commit 0746d15)
…e vector in shared memory in the RobustPrune occlusion loop (cherry picked from commit f45fd1b49283434eb4a3017da069ead501e938c3)
…efix sum with cub scan and hoist per-batch reverse-edge allocations (cherry picked from commit 3b8650f5ebea52421f187332a2f6f3bdd599c42e)
…lusion across multiple warps per query (raise occupancy) (cherry picked from commit 2e02f938f97e65ca073daf07397d538432b52867)
… query->existing-edge distances in the RobustPrune merge (avoid recompute) (cherry picked from commit 041d355c585b7f98302d054b80ac287d637a07e4)
…oords in FP16 smem for dim>=512 to raise GreedySearch occupancy (salvage of N8)
…nce (one warp) instead of redundantly on all 128 threads, then broadcast
…ck (4 vs 8) to raise occupancy/MLP on the degree-64 occlusion sweep
| #define KERNEL_TIMING (RAFT_LOG_ACTIVE_LEVEL <= RAPIDS_LOGGER_LOG_LEVEL_DEBUG) | ||
|
|
||
| template <typename accT, typename IdxT> | ||
| __global__ void gather_query_sizes(QueryCandidates<IdxT, accT>* query_list, |
There was a problem hiding this comment.
Do you have a sense for how much this is adding to the binary size? @divyegala should be able to explain how to see the deployed metrics for PRs.
|
/ok to test c055c60 |
|
/ok to test df13b42 |
| @pytest.mark.parametrize("dtype", [np.float32, np.int8, np.uint8]) | ||
| @pytest.mark.parametrize("dtype", [np.float32, np.float16, np.int8, np.uint8]) | ||
| def test_vamana_build_basic(dtype): | ||
| n_rows, n_cols = 1000, 16 |
There was a problem hiding this comment.
Could we add at least one test at larger dims? The current new FP16 coverage uses only 16 dimensions, so I think we dont test greedy_search_use_fp16_query_smem()?
There was a problem hiding this comment.
This is just for the python test. Added fp16 cpp tests (cpp/tests/neighbors/ann_vamana/test_half_uint32_t.cu) that cover a wide range of params.
|
/ok to test 350ed89 |
@jamxia155, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 74caba9 |
|
/ok to test b8238f3 |
|
|
||
| num_neighbors = degree; | ||
| __syncthreads(); | ||
| num_neighbors[warpIdx] = degree; |
There was a problem hiding this comment.
maybe make only lane 0 write to this?
There was a problem hiding this comment.
Having just lane 0 write it would require a sync after. Seems simpler/cleaner to just have all threads write, since they have the same value.
| __float2half(0.0f), __float2half(0.0f), __float2half(0.0f), __float2half(0.0f)}; | ||
| for (int i = threadIdx.x; i < src_vec->Dim; i += 4 * blockDim.x) { | ||
| temp_dst[0] = dst_vec->coords[i]; | ||
| if (i + 32 < src_vec->Dim) temp_dst[1] = dst_vec->coords[i + 32]; |
There was a problem hiding this comment.
Should we use blockDim.x instead of hardcoding to 32 here?
There was a problem hiding this comment.
I agree with this. We should not be hardcoding things because the kernel invocation can change (or the same kernel may be called somewhere else in the future).
There was a problem hiding this comment.
This is run by a single warp within the block, and uses a warp width stride. I can change it to a define macro if that helps?
There was a problem hiding this comment.
Replaced with raft::WarpSize
tarang-jain
left a comment
There was a problem hiding this comment.
There are a lot of changes in this PR where I want to basically tell the exact same things -- using raft macros and raft primtives wherever possible. We should certainly avoid writing small kernels which can easily be a lambda for raft::linalg. It helps keeps things readable and binary size increases can become slightly more predictable. For example, earlier we have found that raft::linalg::map kernels generally add less to the binary size than thrust::for_each (or similar thrust primitives).
| Node<accT>* topk_pq_mem) | ||
| { | ||
| int n = dataset.extent(0); | ||
| const int warpIdx = threadIdx.x / 32; |
There was a problem hiding this comment.
here as well. Lets not hardcode 32. Lets use raft::WarpSize instead.
There was a problem hiding this comment.
Replaced all warp width values with raft::WarpSize throughout
| Node<SUMTYPE> input_data, | ||
| SUMTYPE* cur_max_val, | ||
| int* max_idx) | ||
| __inline__ __device__ void parallel_pq_max_enqueue_warp(Node<SUMTYPE>* pq, |
There was a problem hiding this comment.
Nit: we have macros to mark kernels: such as _RAFT_INLINE
There was a problem hiding this comment.
I wouldnt say these macros should block the PR from merging.
There was a problem hiding this comment.
I can change this throughout, but I can't find the docs in raft on the different INLINE macros.
| __global__ void scatter_prefix_offsets(QueryCandidates<IdxT, accT>* query_list, | ||
| const int* edge_offsets, | ||
| int count) | ||
| { |
There was a problem hiding this comment.
same here: we should be able to achieve these with raft primitives.
There was a problem hiding this comment.
This scatter kernel is not so simple. I could do it but would need to re-work the code a bit more to use a strided mdspan or something. Can work on this if it's a blocker for the PR.
| // 1024 threads/SM, so 12 * 128 = 1536 exceeds the limit and ptxas emits | ||
| // ".minnctapersm out of range ... will be ignored". Use 8 (8 * 128 = 1024) on those | ||
| // architectures and keep the tuned value of 12 elsewhere (>=1536 threads/SM). | ||
| #if defined(__CUDA_ARCH__) && (__CUDA_ARCH__ == 720 || __CUDA_ARCH__ == 750) |
There was a problem hiding this comment.
This macro can be used only in device code, right? For host code we can use raft::resource::get_device_properties. Fore example we have this elsewhere:
/**
* @brief Returns true if the fused distance NN implementation should be used.
*
* On Ampere (SM <= 8.x) always use fused.
* On Hopper (SM 9.x) use fused when m or n >= 4096.
* On Blackwell (SM >= 10.x) use unfused.
*/
template <typename MathT, typename IdxT, typename LabelT>
bool use_fused(const raft::resources& handle, IdxT m, IdxT n, IdxT k)
{
cudaDeviceProp prop;
prop = raft::resource::get_device_properties(handle);
if (prop.major <= 8) {
// Use fused for Ampere or before
return true;
} else if (prop.major == 9 && (m >= 4096 || n >= 4096)) {
// On Hopper if m, n are bigger than 4096, use fused
return true;
} else if (prop.major >= 10) {
// On Blackwell onwards, use unfused
return false;
}
return false;
}
There was a problem hiding this comment.
This is only used here to define the launch_bounds, since older hardware has a smaller max threads per SM, and using the smaller value on new hardware hurts performance. It's a compile-time parameter so it needs to be a macro, I believe. It looks like the use_fused example determines which kernel to use during runtime, though.
Series of GPU Vamana build performance optimizations that addresses #2178 and #1757. Initial estimates from #1757 were not accurate, so many other optimizations were tried (some abandoned, some successful). This PR includes:
GreedySearch optimizations:
RobustPrune optimizations:
General optimizations:
Together these optimizations give significant speedups across all configs with minimal recall variance compared to the current baseline. I benchmarked performance across a range of synthetic datasets and two real-world datasets. (NOTE: finishing benchmarks and will update tables below once they are all collected).
Synthetic dataset build tests (all 1M vector datasets)
Also tested real-world BIGANN 10M (uint8 128D) and GIST (fp32 960D) datasets: