Skip to content

Reuse Base.ReshapeArray/ReinterpretArray #322

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

Merged
merged 4 commits into from
Sep 23, 2020
Merged

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Sep 22, 2020

Currently, reshape and reinterpret are expected to yield the original array type, complicating that type's definition (it should somehow refcount the underlying buffer, or otherwise support mutating the object). Furthermore, with view we sometimes (in the non-contiguous case) need to return a SubArray anyhow. So let's just get rid of that behavior, re-use Base.ReshapedArray and ReinterpretArray.

As a result, downstream packages (i.e. CUDA.jl, AMDGPU.jl, oneAPI.jl) will need to make sure operations that previously only needed to accept a ::GPUArray now also accept a ReinterpretArray{<:CuArray}, and the same for ReshapeArray. The reference implementation here does so using a DenseJLArray type, for methods that work with (a pointer to) contiguous memory. Next to that, this PR also adds StridedJLArray for supporting methods that accept a pointer and a stride (not used here, but interesting for working with CUBLAS in JuliaGPU/CUDA.jl#435), and we already had WrappedJLArray for types that we can convert to something GPU-compatible (i.e. for kernels).

TL;DR: remove GPUArrays.unsafe_reinterpret, don't implement Base._reshape, add a DenseGPUArray definition and use that with methods that previously accepted ::GPUArray and converted it to a pointer. For bonus points: do the same for view and always have it return a SubArray, introduce a StridedGPUArray type and use it for methods that can work with a pointer+stride, and together with the reshape/reinterpret changes this should allow removing all buffer refcounting from your GPU array type.

cc @jpsamaroo

@timholy
Copy link
Member

timholy commented Sep 22, 2020

xref JuliaLang/julia#37559, in case that meddles in your plans here.

@maleadt
Copy link
Member Author

maleadt commented Sep 23, 2020

Thanks for the link; I think we should be fine if we adapt the Strided/Dense aliases here appropriately. For GPU compatibility, we currently "materialize" wrappers like this when invoking a kernel, so we should be fine.

@maleadt maleadt merged commit ca625b4 into master Sep 23, 2020
@bors bors bot deleted the tb/rm_reshape_reinterpret branch September 23, 2020 06:17
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.

2 participants