JuliaGPU / GPUArrays.jl

Reusable array functionality for Julia's various GPU backends.
MIT License
313 stars 74 forks source link

Define `append!` #533

Closed mtfishman closed 1 month ago

mtfishman commented 1 month ago

This defines a generic version of append! for AbstractGPUVector.

It is similar to the definition for Vector in Base: https://github.com/JuliaLang/julia/blob/6f6bb950b3e7b0a6e77bb11ffd7cc001cb87439d/base/array.jl#L1314-L1320

It relies on resize! and copyto! being defined for the AbstractGPUVector being appended to, while Base relies on Base._growend! and copyto!. Relying on resize! seems reasonable, most of the GPU backends, for example CUDA.jl, Metal.jl, and AMDGPU.jl, have resize! implemented. (EDIT: Support for resize! in oneAPI.jl was added in https://github.com/JuliaGPU/oneAPI.jl/pull/436 but it still needs to be registered).

For now it doesn't support appending with a Tuple of items (like Base allows for Vector) because it appears that copyto! doesn't generally support that for AbstractGPUVectors:

julia> using Metal

julia> v = mtl([1, 2, 3])
3-element MtlVector{Int64, Private}:
 1
 2
 3

julia> copyto!(v, 1, (4, 5, 6), 1, 3)
ERROR: Scalar indexing is disallowed.
Invocation of setindex! 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 should be avoided.

If you want to allow scalar iteration, use `allowscalar` or `@allowscalar`
to enable scalar iteration globally or for the operations in question.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] errorscalar(op::String)
   @ GPUArraysCore ~/.julia/packages/GPUArraysCore/GMsgk/src/GPUArraysCore.jl:155
 [3] _assertscalar(op::String, behavior::GPUArraysCore.ScalarIndexing)
   @ GPUArraysCore ~/.julia/packages/GPUArraysCore/GMsgk/src/GPUArraysCore.jl:128
 [4] assertscalar(op::String)
   @ GPUArraysCore ~/.julia/packages/GPUArraysCore/GMsgk/src/GPUArraysCore.jl:116
 [5] setindex!
   @ ~/.julia/dev/GPUArrays/src/host/indexing.jl:56 [inlined]
 [6] copyto!(dest::MtlVector{Int64, Private}, dstart::Int64, src::Tuple{Int64, Int64, Int64}, sstart::Int64, n::Int64)
   @ Base ./abstractarray.jl:1022
 [7] top-level scope
   @ REPL[16]:1
 [8] top-level scope
   @ ~/.julia/packages/Metal/q9oGt/src/initialization.jl:57

julia> using Pkg; Pkg.status("Metal")
Status `.../Metal.jl/Project.toml`
  [dde4c033] Metal v1.1.0

EDIT: The current PR now supports for appending with a Tuple by collecting it into a Vector, though perhaps it should be collected onto the GPU where the vector that is being appended lives.

tgymnich commented 1 month ago

oneAPI.jl does not seem to have resize! defined.

maleadt commented 1 month ago

This seems like a good idea, but will need some work, e.g., making sure all back-ends actually do implement resize! (oneAPI and JLArrays seem to be missing), making sure there's test coverage by re-enabling the vector tests, etc.

For now it doesn't support appending with a Tuple of items (like Base allows for Vector) because it appears that copyto! doesn't generally support that for AbstractGPUVectors:

Why not convert the Tuple to an Array in the append! implementation for the time being (and add a comment on why it's needed)?

mtfishman commented 1 month ago

I've addressed most comments in the latest commit:

In append!(a::AbstractGPUVector, items::Tuple), should the tuple items get allocated onto the GPU that a lives on?

I can work on oneAPI.jl support for resize! as best as I can but I don't have an Intel GPU available to use. Would that block this from being merged? It seems like the situation for oneAPI.jl before and after this PR is the same, i.e. either way right now resize! and append! don't work for oneArray and it seems like those could be fixed in the future (by implementing resize!(a::oneVector, length::Integer)).

mtfishman commented 1 month ago

I started implementing resize! for oneArray in https://github.com/JuliaGPU/oneAPI.jl/pull/436.