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 |
| } else if constexpr (std::is_same_v<DataT, half>) { | ||
| // raft::random::normal requires std::is_floating_point (excludes half); use uniform, | ||
| // which supports half. uniformInt (the integer branch below) rejects half. | ||
| raft::random::uniform( | ||
| handle_, r, database.data(), ps.n_rows * ps.dim, DataT(0.1), DataT(2.0)); | ||
| raft::random::uniform( | ||
| handle_, r, search_queries.data(), ps.n_queries * ps.dim, DataT(0.1), DataT(2.0)); |
There was a problem hiding this comment.
I suggest that we just generate normal data and then cast that to fp16 instead of generating fp16 in uniform distribution
|
|
||
| num_neighbors = degree; | ||
| __syncthreads(); | ||
| num_neighbors[warpIdx] = degree; |
There was a problem hiding this comment.
maybe make only lane 0 write to this?
| } | ||
|
|
||
| // Checks current list to see if a node as previously been visited | ||
| __inline__ __device__ bool check_visited(IdxT target, accT dist) |
There was a problem hiding this comment.
I believe this function is no longer used anywhere anymore. Should we delete it?
| __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.
| } | ||
|
|
||
| if (threadIdx.x == 31) { | ||
| if (laneId == 31) { |
There was a problem hiding this comment.
Again I am marking more places where things are hardcoded. We have raft variables to take care of this.
There was a problem hiding this comment.
Replace with raft::WarpSize-1
| #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.
I would argue that we can achieve these things with raft linalg primitives (such as raft::linalg::map rather than having to write a new kernel).
| __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.
| } | ||
|
|
||
| template <typename T> | ||
| __host__ __device__ inline int greedy_search_query_smem_elem_size(int dim) |
There was a problem hiding this comment.
Again, lets use our raft macros: RAFT_HOST_DEVICE_INLINE
| // Find which node IDs have reverse edges and their indices in the reverse edge list | ||
| thrust::device_vector<IdxT> edge_dest_vec(edge_dest.data_handle(), | ||
| edge_dest.data_handle() + total_edges); | ||
| RAFT_CUDA_TRY(cudaMemcpyAsync(thrust::raw_pointer_cast(edge_dest_vec.data()), |
| int accept_count = 0; | ||
| if (s_res_size > degree) { | ||
| if (threadIdx.x == 0) s_accept_count = 0; | ||
| __syncthreads(); |
There was a problem hiding this comment.
do we need to sync the whole block here? We do it already at line 234. I see that s_accept_count is set to zero and you might need the whole block to see this before moving forward, but how about all threads set that variable rather than just thread zero setting it? Let me know if my understanding is not correct here.
There was a problem hiding this comment.
All the threads in the block use the shared s_accept_count after this, and any thread may increment it. If we have all threads set it to 0 and no sync, we may lose later increments by a trailing thread/warp re-setting it to 0.
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: