Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define append! #533

Merged
merged 4 commits into from
May 24, 2024
Merged

Define append! #533

merged 4 commits into from
May 24, 2024

Conversation

mtfishman
Copy link
Contributor

@mtfishman mtfishman commented May 8, 2024

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 JuliaGPU/oneAPI.jl#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
Copy link
Member

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

test/testsuite/vector.jl Outdated Show resolved Hide resolved
@maleadt
Copy link
Member

maleadt commented May 13, 2024

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
Copy link
Contributor Author

mtfishman commented May 14, 2024

I've addressed most comments in the latest commit:

  • I re-enabled "testsuite/vector.jl" and deleted tests that looked outdated.
  • I added support for append!(::AbstractGPUVector, ::Tuple).
  • I defined resize! for JLArray.

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
Copy link
Contributor Author

I started implementing resize! for oneArray in JuliaGPU/oneAPI.jl#436.

src/host/abstractarray.jl Outdated Show resolved Hide resolved
@maleadt maleadt merged commit ec9fe5b into JuliaGPU:master May 24, 2024
14 checks passed
@mtfishman mtfishman deleted the append branch May 24, 2024 10:34
@mtfishman mtfishman mentioned this pull request Jun 12, 2024
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.

3 participants