[FEA] Add support for SDDMM by wrapping the cusparseSDDMM (#2067) #2067

rhdong · 2023-12-18T01:52:25Z

Add support for SDDMM by wrapping the cusparseSDDMM
This PR also moved some APIs shared with SpMM to the utils.cuh file.

copy-pr-bot · 2023-12-18T01:52:28Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

benfred · 2023-12-18T18:16:55Z

/ok to test

- Add support for SDDMM by wrapping the `cusparseSDDMM` - This PR also moved some APIs shared with `SpMM` to the `utils.cuh` file. Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) URL: #2067

benfred · 2023-12-18T19:13:32Z

/ok to test

- Add support for SDDMM by wrapping the `cusparseSDDMM` - This PR also moved some APIs shared with `SpMM` to the `utils.cuh` file. Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) URL: #2067

benfred

This looks great - thanks for the PR!

cpp/include/raft/sparse/linalg/sddmm.cuh

rhdong · 2023-12-18T23:00:28Z

/ok to test

benfred · 2023-12-18T23:34:26Z

/ok to test

- Add support for SDDMM by wrapping the `cusparseSDDMM` - This PR also moved some APIs shared with `SpMM` to the `utils.cuh` file. Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) URL: #2067

benfred

lgtm!

lowener · 2023-12-19T11:47:54Z

cpp/include/raft/sparse/linalg/sddmm.cuh

+          typename NZType,
+          typename LayoutPolicyA,
+          typename LayoutPolicyB>
+void sddmm(raft::resources const& handle,


For consistency with the RAFT API functions, the order of parameters should be:

Handle

Input view

Output view

Extra parameter (alpha, beta, trans_a, trans_b)

(Even though spmm currently doesn't do that)
And using raft::host_scalar_view should be a good idea instead of a raw pointer for alpha and beta.

That makes sense! But considering the minimum of the customer's learning cost, may I suggest here to keep the params in a similar sequence with the cuSparse original API？

The RAFT APIs don't assume the user is familiar with, nor even knows that cusparse is being used under the hood, so it's best to keep up with the conventions established by RAFT so that the user has a consistent experience.

@lowener is correct to point this out- we had a lot of discussions about this when the convention was established and we strive to use the same conventions everywhere.

(Resources, param structs, in, out, params)

Our APIs are also intentionally based on mdspan so we shouldn't accept pointers anywhere. All of the pointer-based APIs that are exposed publicly are deprecated.

If alpha and beta can be specified on device or host, we should capture this with mdspans. This also makes the APIs self documenting.

lowener · 2023-12-19T11:56:57Z

cpp/include/raft/sparse/linalg/detail/utils.cuh

+          typename IndexType,
+          typename LayoutPolicyA,
+          typename LayoutPolicyB>
+bool is_row_major(raft::device_matrix_view<ValueTypeA, IndexType, LayoutPolicyA>& a,


This function can be constexpr. Can it also reuse raft::util::is_row_major ?

Accept, and I removed it because the new implement doesn't need the API anymore.

- Add support for SDDMM by wrapping the `cusparseSDDMM` - This PR also moved some APIs shared with `SpMM` to the `utils.cuh` file. Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) URL: #2067

rhdong · 2024-01-06T01:54:41Z

Hi @cjnolet @benfred @lowener, I updated the code inspired by our discussion. Please help review it if you have time. Thank you!

lowener · 2024-01-08T17:12:24Z

cpp/include/raft/linalg/linalg_types.hpp

@@ -32,4 +32,11 @@ enum class Apply { ALONG_ROWS, ALONG_COLUMNS };
 */
 enum class FillMode { UPPER, LOWER };

+/**
+ * @brief Enum for this type indicates which operations is applied to the related input (e.g. sparse


lowener · 2024-01-08T17:16:44Z

cpp/include/raft/sparse/linalg/detail/utils.cuh

+ */
+template <typename ValueType, typename IndexType, typename LayoutPolicy>
+cusparseDnMatDescr_t create_descriptor(
+  raft::device_matrix_view<ValueType, IndexType, LayoutPolicy>& dense_view, const bool is_row_major)


Views/mdspans are usually passed by value and not by reference because they are lightweight.

is_row_major is not necessary here because this information can be inferred from the layout of the matrix view. Call raft::is_row_major() inside this function.

Accept, just inherit from the spmm.

lowener · 2024-01-08T17:26:40Z

cpp/include/raft/sparse/linalg/detail/utils.cuh

+ * @brief convert the operation to cusparseOperation_t type
+ * @tparam OpVal type of operation
+ */
+static inline cusparseOperation_t convert_operation(const raft::linalg::Operation& op)


Does this function need to be static? The reference is not needed as well, this can be passed by value.

lowener · 2024-01-08T18:10:44Z

cpp/include/raft/sparse/linalg/detail/utils.cuh

+
+/**
+ * @brief convert the operation to cusparseOperation_t type
+ * @tparam OpVal type of operation


Update this comment: param[in] op type of operation

- Add support for SDDMM by wrapping the `cusparseSDDMM` - This PR also moved some APIs shared with `SpMM` to the `utils.cuh` file. Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) URL: #2067

cjnolet · 2024-01-09T03:31:16Z

@rhdong Why so many force pushes to the branch? You should be able to merge upstream into your branch cleanly. The commits are squashed automatically upon merging the PR so there's no reason to rewrite history.

rhdong · 2024-01-09T04:00:59Z

@rhdong Why so many force pushes to the branch? You should be able to merge upstream into your branch cleanly. The commits are squashed automatically upon merging the PR so there's no reason to rewrite history.

OK, get it~(just want to make the PR as one commit and keep the history clear)

cjnolet

This looks great so far! I mostly have minor things (in addition to automating the expected test data and contributing the benchmarks that you've put a lot of effort into).

cjnolet · 2024-01-09T18:47:45Z

cpp/include/raft/sparse/linalg/detail/sddmm.hpp

+                                                   &bufferSize,
+                                                   resource::get_cuda_stream(handle)));
+
+  raft::interruptible::synchronize(resource::get_cuda_stream(handle));


We're trying to centralize the interruptible calls instead of calling it directly, please use resource::sync_stream() instead.

cjnolet · 2024-01-09T18:49:21Z

cpp/include/raft/sparse/linalg/detail/utils.cuh

@@ -0,0 +1,103 @@
+/*


Just to avoid confusion, can we rename this file to cusparse_utils.hpp? (Please note this shouldn't be a cuh because it's not creating any device functions).

cjnolet · 2024-01-09T18:51:07Z

cpp/include/raft/sparse/linalg/sddmm.cuh

@@ -0,0 +1,83 @@
+/*


Since this file doesn't create or use any device functions, please rename to sddmm.hpp. This is a great designation to users that it only reuires the CUDA runtime APIs, math libs, and nothing else.

cjnolet · 2024-01-09T18:52:10Z

cpp/include/raft/sparse/linalg/spmm.cuh

@@ -19,6 +19,7 @@
 #pragma once



This file should probaly be hpp as well. Up to you whether you want to rename it in this PR (since it's already quite big).

cjnolet · 2024-01-09T18:52:33Z

cpp/test/sparse/sddmm.cu

@@ -0,0 +1,425 @@
+/*
+ * Copyright (c) 2023-2024, NVIDIA CORPORATION.


New file should only have current year

cjnolet · 2024-01-09T18:52:49Z

cpp/include/raft/sparse/linalg/sddmm.cuh

@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2023-2024, NVIDIA CORPORATION.


New files should only have current year.

cjnolet · 2024-01-09T18:53:11Z

cpp/include/raft/sparse/linalg/detail/utils.cuh

@@ -0,0 +1,103 @@
+/*
+ * Copyright (c) 2023-2024, NVIDIA CORPORATION.


New files should only have current year

cjnolet · 2024-01-09T18:53:30Z

cpp/include/raft/sparse/linalg/detail/sddmm.hpp

@@ -0,0 +1,99 @@
+/*
+ * Copyright (c) 2023-2024, NVIDIA CORPORATION.


New files should only have current year

cjnolet · 2024-01-09T22:52:58Z

cpp/test/sparse/sddmm.cu

+    4,
+    4,
+    3,
+    1.0,


There are a few places where we do this because it's really hard to generate the outputs for automating the comparisons. The problem with hardcoding these things is that it's non-trivial to add new test cases and so when a user comes comes to us with an issue where they are seeing strange results with some sparsities or specific inputs, we couldn't otherwise be able to quickly reproduce in a test once we fix the issue.

I think it's easy enough to automate theexpected outputs, though- since you are already creating a mask, just copy out the code from your micro benchmarks that creates a random mask, generate two input dense arrays (using raft::random::make_blobs), compute the pairwise distances between A and B, and then copy in the rows/cols of the results to your "expected output" parse structure.

Given how much work went into benchmarking, I'd also highly suggest we commit your benchmarking code with these changes also. Since it was already written, I can't express enough how convenient it is to be able to load up a simple microbenchmark when someone finds a specific case to test. I understand this version is a lightweight wrapper around the SDDMM from cusparse, but that's likely not always going to be the case and so automated tests and benchmarks helps us evolve the code over time (and back-ends).

Make sense!

cjnolet · 2024-01-12T01:12:27Z

cpp/test/sparse/sddmm.cu

+                                                   ValueType(1.0f),
+                                                   uint64_t(2024));
+
+    raft::copy(a_data_h.data(), blobs_a_b.data_handle(), a_size, stream);


The new tests look great, thanks!

cjnolet · 2024-01-12T01:16:17Z

cpp/include/raft/sparse/linalg/spmm.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, NVIDIA CORPORATION.
+ * Copyright (c) 2023-2024, NVIDIA CORPORATION.


Since this is a publicly facing header, we should add a new file called spmm.cuh and have it import this header (with an include guard, of course), so that it doesn't break users downstream.

Make sense.

) - Add support for SDDMM by wrapping the `cusparseSDDMM` - This PR also moved some APIs shared with `SpMM` to the `utils.cuh` file. Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#2067

cjnolet

LGTM! Thanks @rhdong!

cjnolet · 2024-01-16T21:00:38Z

/merge

rhdong requested review from a team as code owners December 18, 2023 01:52

github-actions bot added cpp CMake labels Dec 18, 2023

rhdong requested a review from benfred December 18, 2023 01:52

rhdong force-pushed the rhdong/sddmm branch from f20dec2 to b65e989 Compare December 18, 2023 01:53

rhdong requested a review from cjnolet December 18, 2023 01:55

rhdong force-pushed the rhdong/sddmm branch from b65e989 to 85e3ff7 Compare December 18, 2023 01:59

rhdong added feature request New feature or request non-breaking Non-breaking change labels Dec 18, 2023

rhdong force-pushed the rhdong/sddmm branch from 85e3ff7 to 7318282 Compare December 18, 2023 04:13

rhdong changed the title ~~[FEA] Add support for SDDMM by wrapping the cusparseSDDMM (#2xxx)~~ [FEA] Add support for SDDMM by wrapping the cusparseSDDMM (#2067) Dec 18, 2023

rhdong force-pushed the rhdong/sddmm branch from 7318282 to 182c3fd Compare December 18, 2023 18:26

benfred reviewed Dec 18, 2023

View reviewed changes

cpp/include/raft/sparse/linalg/sddmm.cuh Outdated Show resolved Hide resolved

rhdong force-pushed the rhdong/sddmm branch from 182c3fd to cbaba37 Compare December 18, 2023 22:59

benfred added improvement Improvement / enhancement to an existing function and removed feature request New feature or request labels Dec 18, 2023

rhdong force-pushed the rhdong/sddmm branch 2 times, most recently from 13b6a45 to 61d9558 Compare December 19, 2023 00:38

benfred approved these changes Dec 19, 2023

View reviewed changes

lowener reviewed Dec 19, 2023

View reviewed changes

rhdong force-pushed the rhdong/sddmm branch from dc65b1c to 8016277 Compare December 20, 2023 06:02

cjnolet assigned rhdong Jan 3, 2024

rhdong force-pushed the rhdong/sddmm branch 2 times, most recently from 523f4f7 to cdae2b5 Compare January 6, 2024 01:48

lowener reviewed Jan 8, 2024

View reviewed changes

rhdong force-pushed the rhdong/sddmm branch from cdae2b5 to 234c09d Compare January 9, 2024 00:28

rhdong force-pushed the rhdong/sddmm branch from 234c09d to 046e0d7 Compare January 9, 2024 01:44

cjnolet requested changes Jan 9, 2024

View reviewed changes

cjnolet reviewed Jan 12, 2024

View reviewed changes

rhdong added 5 commits January 12, 2024 16:03

fix doc build CI error

e8ae6df

Optimize based on review comments

8bb62d6

add benchmark for SDDMM

4563c3b

add back the spmm.cuh for better compatibility

e922336

rhdong force-pushed the rhdong/sddmm branch from 4363a74 to e922336 Compare January 13, 2024 00:03

rhdong requested review from cjnolet and lowener January 15, 2024 19:39

cjnolet approved these changes Jan 16, 2024

View reviewed changes

rapids-bot bot merged commit 3c7586f into rapidsai:branch-24.02 Jan 16, 2024
61 checks passed

mfoerste4 mentioned this pull request Feb 7, 2024

[BUG] fix row/col-major selection for strided layouts #2166

Closed

		@@ -0,0 +1,425 @@
		/*
		* Copyright (c) 2023-2024, NVIDIA CORPORATION.

		@@ -0,0 +1,83 @@
		/*
		* Copyright (c) 2023-2024, NVIDIA CORPORATION.

		@@ -0,0 +1,103 @@
		/*
		* Copyright (c) 2023-2024, NVIDIA CORPORATION.

		@@ -0,0 +1,99 @@
		/*
		* Copyright (c) 2023-2024, NVIDIA CORPORATION.

+,
+,
+,
+.0,

[FEA] Add support for SDDMM by wrapping the cusparseSDDMM (#2067) #2067

[FEA] Add support for SDDMM by wrapping the cusparseSDDMM (#2067) #2067

Conversation

rhdong commented Dec 18, 2023 • edited by benfred Loading

copy-pr-bot bot commented Dec 18, 2023

benfred commented Dec 18, 2023

benfred commented Dec 18, 2023

benfred left a comment

Choose a reason for hiding this comment

rhdong commented Dec 18, 2023

benfred commented Dec 18, 2023

benfred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjnolet Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhdong commented Jan 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhdong Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjnolet commented Jan 9, 2024 • edited Loading

rhdong commented Jan 9, 2024

cjnolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjnolet left a comment

Choose a reason for hiding this comment

cjnolet commented Jan 16, 2024

rhdong commented Dec 18, 2023 •

edited by benfred

Loading

cjnolet Dec 19, 2023 •

edited

Loading

rhdong Jan 8, 2024 •

edited

Loading

cjnolet commented Jan 9, 2024 •

edited

Loading