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

Support pointer(A::PermDimsArray) if parent(A) supports it #20385

Merged
merged 1 commit into from
Feb 4, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Feb 2, 2017

Useful for wrapping certain C routines, where the storage order differs from Julia's.

@stevengj
Copy link
Member

stevengj commented Feb 2, 2017

Don't you also need strides, so that the pointer can be used? Or is that already implemented?

@timholy
Copy link
Member Author

timholy commented Feb 2, 2017

It seems like we have to have strides for internal consistency, and the fallback gives the wrong answer. (I think I've said elsewhere that I suspect we should deprecate the fallback definitions...) So good catch.

However, if you're calling a C routine that wants one memory layout and Julia wants a different one, you will likely want to pass the C routine strides(parent(P)) rather than strides(P) (where P is the PDA). In other words, say you want to fill a matrix from a C routine that expects row-major layout. Then it will look like this:

function foo{T}(::Type{T}, width, height)
    A = Matrix{T}(width, height)   # not height, width
    ccall(:fill_matrix, Void, (blah blah), A, strides(A))
    PermutedDimsArray(A, (2,1))
end

You wouldn't want to pass strides(P) here.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

@timholy, if you have a C routine that accepts a pointer and sizes and strides, surely you would want the permuted strides if you are passing the permuted dims? e.g. suppose you have

# call C function: void init_matrix(double*a, int n1, int n2, int stride1, int stride2)
init_matrix!(M::AbstractMatrix{Cdouble}) =
     ccall(:init_matrix, Void, (Ptr{Cdouble},Cint,Cint,Cint,Cint), M, size(M)..., strides(M)...)

and then you want to pass A = PermutedDimsArray(rand(3,4), (2,1)) to init_matrix!.

  • size(A) returns (4,3).
  • strides(A) should return the strides of the underlying data along these dimensions (in the same order!): (3,1) ... that is, from C's perspective it is a 4x3 row-major array
  • pointer(A) should return the underlying pointer
  • unsafe_convert(Ptr{Cdouble}, A) should return the underlying pointer

@timholy
Copy link
Member Author

timholy commented Feb 3, 2017

Yes, all that's fine. But if the C code is flexible enough to accept general strides, I don't understand why you wouldn't want to use an Array and "lie" about the size and strides. When it comes to ccalls, PDA's main application is to handle cases where the C code doesn't have that kind of flexibility.

In any event, this already implements all of your list except unsafe_convert(Ptr{T}, A). The reason is that this is what's used to implement pointer(A, i), and as I noted in the comments, for PDAs that's a dangerous thing to support. But an alternative would be to implement unsafe_convert and specialize pointer(::PDA, i) to throw an error. Now that I think about it, that seems slightly better (it avoids the need to manually call pointer in the ccall), so I'll revise this accordingly.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

@timholy, I was thinking of generic code that accepts any AbstractArray, and you happen to have a permuted-dims array for some other reason that you want to pass.

@timholy timholy merged commit cc892c0 into master Feb 4, 2017
@timholy timholy deleted the teh/pd_pointer branch February 4, 2017 15:32
tkelman pushed a commit that referenced this pull request Mar 1, 2017
…s them

(cherry picked from commit 2bc83ca)
ref #20385

Qualify PermutedDimsArray as it isn't exported on release-0.5
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