Skip to content

Update CUSPARSE.sum_dim<n> to allow for arbitrary function on elements - #1987

Merged
maleadt merged 3 commits into
JuliaGPU:masterfrom
lpawela:lp/sum-dim-fix
Jul 18, 2023
Merged

maleadt merged 3 commits into
JuliaGPU:masterfrom
lpawela:lp/sum-dim-fix

Conversation

@lpawela

@lpawela lpawela commented Jul 1, 2023 •

Copy link
Copy Markdown
Contributor

This fixes #1983

EDIT: also fixes #1406

@maleadt

maleadt commented Jul 2, 2023

Copy link
Copy Markdown
Member

sum_dim1 etc aren't public functions, so I don't see why we would want to add configurability without a public API to use it.

Why are you calling these functions directly?

@lpawela

lpawela commented Jul 2, 2023

Copy link
Copy Markdown
Contributor Author

The main reason is scalar indexing when using sum(ad, dims=2)

julia> using CUDA, SparseArrays

julia> a = sprand(10, 10, 0.1);

julia> ad = CUDA.CUSPARSE.CuSparseMatrixCSR(a);

julia> sum(ad, dims=2)
┌ Warning: Performing scalar indexing on task Task (runnable) @0x00007f2882ffc010.
│ Invocation of getindex resulted in scalar indexing of a GPU array.
│ This is typically caused by calling an iterating implementation of a method.
│ Such implementations *do not* execute on the GPU, but very slowly on the CPU,
│ and therefore are only permitted from the REPL for prototyping purposes.
│ If you did intend to index this array, annotate the caller with @allowscalar.
└ @ GPUArraysCore ~/.julia/packages/GPUArraysCore/uOYfN/src/GPUArraysCore.jl:106
10×1 Matrix{Float64}:
 0.7312257183333077
 0.2736255562180383
 0.0
 0.8569093552724679
 0.24512118904635305
 0.0
 0.0
 0.0
 0.0
 0.9815900426493798

@maleadt

maleadt commented Jul 5, 2023

Copy link
Copy Markdown
Member

Right, but then we should hook up sum to these functions so that you don't have to rely on internals.

@lpawela

lpawela commented Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

I just noticed that the sum_dim1 and sum_dim2 seem to be inverted compared to Base.sum

julia> using CUDA, SparseArrays

julia> A = sprand(4, 4, 0.8);

julia> dA = CUDA.CUSPARSE.CuSparseMatrixCSR(A);

julia> CUDA.CUSPARSE.sum_dim1(dA)
4-element CuArray{Float64, 1, CUDA.Mem.DeviceBuffer}:
 2.2032459284126857
 1.2487818875938437
 0.7308272178094658
 1.9313369732308794

julia> sum(A, dims=1)
1×4 Matrix{Float64}:
 1.40888  1.72323  0.602062  2.38003

julia> sum(A, dims=2)
4×1 Matrix{Float64}:
 2.2032459284126857
 1.2487818875938437
 0.7308272178094658
 1.9313369732308794

All my previous inputs were symmetric, so it didn't show. Am I missing something or is this a bug?

@maleadt

maleadt commented Jul 12, 2023

Copy link
Copy Markdown
Member

Yeah, looks like the functions are named incorrectly.

@maleadt

maleadt commented Jul 18, 2023

Copy link
Copy Markdown
Member

Hmm, I still don't like this. Those internal functions aren't meant to be reused. As I mention, this should be implemented as Base.sum.

I've tried something like that, reusing the device iterators we have for the native broadcast. As a result, this now supports most of mapreduce, including a full reduction. Could still be improved, of course (e.g., adding in-place support, generalizing how it handles dims, etc).

@maleadt
maleadt merged commit 7660edc into JuliaGPU:master Jul 18, 2023
@lpawela
lpawela deleted the lp/sum-dim-fix branch March 20, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CUSPARSE.sum_dim1 sums the absolute values of elements CUSPARSE does not support reductions

2 participants