JuliaGPU / CUDA.jl

CUDA programming in Julia.
https://juliagpu.org/cuda/
Other
1.21k stars 222 forks source link

[CUSPARSE] Implement a sparse GEMV for CuSparseMatrixCSC * CuSparseVector #2488

Closed amontoison closed 2 months ago

amontoison commented 2 months ago

2484

I didn't added new tests because the current ones should check if the result is correct or not: https://github.com/JuliaGPU/CUDA.jl/blob/master/test/libraries/cusparse/interfaces.jl#L267-L282

Note that a sparse GEMV CuSparseMatrixCSR * CuSparseVector is also implemented.

maleadt commented 2 months ago

I didn't added new tests because the current ones should check if the result is correct or not: https://github.com/JuliaGPU/CUDA.jl/blob/master/test/libraries/cusparse/interfaces.jl#L267-L282

* won't ever dispatch to this version of gemv, right? Instead we convert to a dense vector in generic_matvecmul!. Maybe that method also ought to document why we do so (IIUC because the result likely being dense it's likely more performant to dispatch to sparse*dense instead).

amontoison commented 2 months ago

I didn't added new tests because the current ones should check if the result is correct or not: https://github.com/JuliaGPU/CUDA.jl/blob/master/test/libraries/cusparse/interfaces.jl#L267-L282

* won't ever dispatch to this version of gemv, right? Instead we convert to a dense vector in generic_matvecmul!. Maybe that method also ought to document why we do so (IIUC because the result likely being dense it's likely more performant to dispatch to sparse*dense instead).

* should dispatch to this version of gemv but not the in-place version (mul!): https://github.com/JuliaGPU/CUDA.jl/pull/2488/files#diff-649f5e2f535605fafb1bf53946f4393ce331f181de4c6a2266a86700f2f5d515R190

In practice, we don't know the sparsity pattern of the output and thus for very rare cases we want to do that in-place.

Maybe I should remove the high-level *(CuSparseMatrixCSC, CuSparseVector) and specify that we have a sparse gemv available for advanced users?

maleadt commented 2 months ago

Maybe I should remove the high-level *(CuSparseMatrixCSC, CuSparseVector) and specify that we have a sparse gemv available for advanced users?

I'll leave that up to you to decide what's the most likely scenario here. Adding a docstring on the behavior and the alternative is probably a good idea though.

amontoison commented 2 months ago

I'll leave that up to you to decide what's the most likely scenario here. Adding a docstring on the behavior and the alternative is probably a good idea though.

The most likely scenario is that the result will be dense, so I decided to remove the high-level dispatch for this sparse gemv. Because of that, I added a docstring for gemv and unit tests to verify that gemv is working as intended.