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

Implement convert between Array and CuArray like unsafe_wrap #2434

Open
Jutho opened this issue Jul 3, 2024 · 4 comments
Open

Implement convert between Array and CuArray like unsafe_wrap #2434

Jutho opened this issue Jul 3, 2024 · 4 comments
Labels
cuda array Stuff about CuArray. hard This is difficult.

Comments

@Jutho
Copy link
Contributor

Jutho commented Jul 3, 2024

I am not sure if this qualifies as a bug or a feature request, but I was wondering whether the constructor CuArray{T,N,CUDA.HostMemory}(A::Array{T,N}) should unsafe_wrap(CuArray, A)?

The current general implementation at https://github.com/JuliaGPU/CUDA.jl/blob/master/src/array.jl#L404 will allocate new host memory and then copy the contents of A into this. Because of this, also convert(CuArray{T,N,CUDA.HostMemory}, A::Array{T,N}) (over at GPUArrays.jl) and ultimately adapt(CuArray{T,N,CUDA.HostMemory}, A::Array{T,N}) will result in new host memory being allocated. Hence, the only way to actually reuse A is via unsafe_wrap(CuArray, A), but that does only work for A::Array and not for any of the wrapper types that one could feed into adapt.

@Jutho Jutho added the bug Something isn't working label Jul 3, 2024
@Jutho
Copy link
Contributor Author

Jutho commented Jul 3, 2024

On second thought, I assume Julia's array constructors are typically assumed to make a copy of the incoming data. But probably convert or adapt(CuArray{T,N,CUDA.HostMemory}, A::Array{T,N}) could be special cased to return unsafe_wrap(CuArray, A).

In the same way, adapt(Array, A::CuArray{T,N,<:Union{CUDA.HostMemory,CUDA.UnifiedMemory}) could be special cased to return unsafe_wrap(Array, A)?

@maleadt
Copy link
Member

maleadt commented Jul 4, 2024

I would agree that convert could be made to work like this, while constructors probably should remain copying. However, there's a reason this operation is currently marked unsafe: We can't track the owning memory object, so when you e.g. unsafe_wrap an Array as a CuArray, and the Array is GC'd, the CuArray becomes invalid too. Without that, I don't think we can reasonably make convert call / behave like unsafe_wrap.

EDIT: Thinking about this some more, I'm not entirely convinced if convert or adapt should result in an aliased object. Are there precedents in Base or packages?

@maleadt maleadt added enhancement New feature or request cuda array Stuff about CuArray. and removed bug Something isn't working labels Jul 4, 2024
@maleadt maleadt changed the title Should CuArray{T,N,CUDA.HostMemory}(A::Array{T,N}) equal unsafe_wrap(CuArray, A)? Implement convert between Array and CuArray like unsafe_wrap Jul 4, 2024
@maleadt maleadt added the speculative Not sure about this one yet. label Jul 4, 2024
@huiyuxie
Copy link
Contributor

I'm not entirely convinced if convert or adapt should result in an aliased object. Are there precedents in Base or packages?

Alia occurs only when the original type and the converted type are the same - see https://github.com/JuliaLang/julia/blob/8f5b7ca12ad48c6d740e058312fc8cf2bbe67848/base/essentials.jl#L439-L461

Making convert behave like unsafe_wrap would be odd for me, but the extra and useless host memory allocation inside the constructor is bad - maybe extending the copyto! would work to avoid extra memory allocation but I have seen discussions about extending copyto! to views might not be a good idea for reasons I can't remember now.

@maleadt
Copy link
Member

maleadt commented Oct 22, 2024

Alia occurs only when the original type and the converted type are the same - see https://github.com/JuliaLang/julia/blob/8f5b7ca12ad48c6d740e058312fc8cf2bbe67848/base/essentials.jl#L439-L461

The docstring is much more general:

convert(T, x)

If T is a collection type and x a collection, the result of convert(T, x) may alias all or part of x.

So using unsafe_wrap for convert(::CuArray, ::Array) (or vice versa) would be fine. The only remaining problem is the lifetime tracking, making it currently impossible to do so. For the Array -> CuArray case, we could probably make the managed memory wrapper also take an Array root. The inverse is much more difficult, which is too bad given that it's probably the more useful case.

@maleadt maleadt added hard This is difficult. and removed enhancement New feature or request speculative Not sure about this one yet. labels Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda array Stuff about CuArray. hard This is difficult.
Projects
None yet
Development

No branches or pull requests

3 participants