Skip to content
This repository has been archived by the owner on May 27, 2021. It is now read-only.

Allow transfer of array views backed by contiguous memory (fix #76) #77

Merged
merged 3 commits into from
Jan 10, 2018
Merged

Allow transfer of array views backed by contiguous memory (fix #76) #77

merged 3 commits into from
Jan 10, 2018

Conversation

samuelpowell
Copy link
Contributor

Ref #76

@samuelpowell
Copy link
Contributor Author

This approach seems to work okay, but the use case may be a bit niche.

@maleadt if you have an appetite to merge this I will add tests.

src/array.jl Outdated
if length(dst) != length(src)
throw(ArgumentError("Inconsistent array length."))
end
if any(strides(src) .!= 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail for any multidimensional array, where even a dense array has a stride bigger than one along higher dimensions.

strides is not entirely safe yet, but see JuliaLang/julia#25321 (which should make it safe). If you rely on an unsafe strides, you might get wrong answers or segfaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timholy good point, thanks. I was concentrating on the particular problem I have in #76 and hadn't properly considered this.

I could fix this to work for views which only restrict the trailing dimension to a contiguous region, but that's too peculiar. Alternatively, I could restrict the method only to operate on views which result in a vector, which would still be useful.

That said, if strides isn't safe, then that might be a showstopper. Feel free to close the PR if that's the case - I can always implement these methods privately in my package until things stabilise.

@maleadt
Copy link
Member

maleadt commented Jan 5, 2018

@maleadt if you have an appetite to merge this I will add tests.

No objections per se, but since we're transitioning away from CUDAdrv.CuArrays (just waiting for 0.7 to stabilize and be supported by GPUArrays) it might be better to add this to CuArrays.jl.
But if you'd like this functionality to be in CUDAdrv for a couple of releases still, feel free to push this forward 🙂

@samuelpowell
Copy link
Contributor Author

I haven't yet started the transition to 0.7, so it would be useful to me if this were available here whilst I am still working on 0.6/updating packages. I suspect a more generic/well planned interface which takes account of various array types would be better for the future, and have seen a few related issues and PRs at CuArrays.jl.

For the moment, I have updated the PR to attempt to ensure that it only attempts to copy views which represent a contiguous memory backed region, and added some tests accordingly. @timholy does this address your concerns, or is it still problematic?

@samuelpowell samuelpowell changed the title WIP: Allow transfer of unit-stride array views WIP: Allow transfer of array views backed by contiguous memory Jan 9, 2018
@timholy
Copy link
Member

timholy commented Jan 9, 2018

Yes, this seems safe.

@samuelpowell samuelpowell changed the title WIP: Allow transfer of array views backed by contiguous memory Allow transfer of array views backed by contiguous memory Jan 9, 2018
@samuelpowell samuelpowell changed the title Allow transfer of array views backed by contiguous memory Allow transfer of array views backed by contiguous memory (fix #76) Jan 9, 2018
@samuelpowell
Copy link
Contributor Author

Thanks @timholy

@maleadt
Copy link
Member

maleadt commented Jan 10, 2018

Thanks. CI failure unrelated, probably because this PR isn't based on master.

@maleadt maleadt merged commit bbb3ae7 into JuliaGPU:master Jan 10, 2018
@rveltz
Copy link

rveltz commented Jul 5, 2018

Hi,

I am having a similar problem. Is it really solved?

julia> phi0 , flag = @time IterativeSolvers.gmres(Jl,v,tol = 1e-6,log = true,verbose=true,restart=200)
ERROR: conversion to pointer not defined for CuArray{Float32,1}
Stacktrace:
 [1] copy!(::SubArray{Float32,1,Array{Float32,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/GPUArrays/src/abstractarray.jl:120
 [2] #init!#55(::Bool, ::Function, ::IterativeSolvers.ArnoldiDecomp{Float32,LinearMaps.FunctionMap{Float32,JacobianNF,Void}}, ::Array{Float32,1}, ::CuArray{Float32,1}, ::IterativeSolvers.Identity, ::Array{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:219
 [3] (::IterativeSolvers.#kw##init!)(::Array{Any,1}, ::IterativeSolvers.#init!, ::IterativeSolvers.ArnoldiDecomp{Float32,LinearMaps.FunctionMap{Float32,JacobianNF,Void}}, ::Array{Float32,1}, ::CuArray{Float32,1}, ::IterativeSolvers.Identity, ::Array{Float32,1}) at ./<missing>:0
 [4] #gmres_iterable!#52(::IterativeSolvers.Identity, ::IterativeSolvers.Identity, ::Float64, ::Int64, ::Int64, ::Bool, ::Function, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:118
 [5] (::IterativeSolvers.#kw##gmres_iterable!)(::Array{Any,1}, ::IterativeSolvers.#gmres_iterable!, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at ./<missing>:0
 [6] #gmres!#54(::IterativeSolvers.Identity, ::IterativeSolvers.Identity, ::Float64, ::Int64, ::Int64, ::Bool, ::Bool, ::Bool, ::IterativeSolvers.#gmres!, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:185
 [7] (::IterativeSolvers.#kw##gmres!)(::Array{Any,1}, ::IterativeSolvers.#gmres!, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at ./<missing>:0
 [8] #gmres#53(::Array{Any,1}, ::Function, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:134
 [9] (::IterativeSolvers.#kw##gmres)(::Array{Any,1}, ::IterativeSolvers.#gmres, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at ./<missing>:0

@samuelpowell
Copy link
Contributor Author

It was certainly working for me at the time, though I haven't used it recently. I suggest opening an issue with a minimum working example and xref accordingly.

@rveltz
Copy link

rveltz commented Jul 7, 2018 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants