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

PermutedDimsArray #10

Closed
mcabbott opened this issue Apr 2, 2020 · 7 comments · Fixed by #12
Closed

PermutedDimsArray #10

mcabbott opened this issue Apr 2, 2020 · 7 comments · Fixed by #12

Comments

@mcabbott
Copy link
Contributor

mcabbott commented Apr 2, 2020

Does this package plan to support higher-dimensional arrays? And if so, what should the behaviour be on PermutedDimsArray? Right now:

julia> MemoryLayout(ones(10,20,30)  |> typeof)
DenseColumnMajor()

julia> MemoryLayout(PermutedDimsArray(ones(10,20,30), (1,3,2))  |> typeof)
UnknownLayout()

My context for this is JuliaGPU/CuArrays.jl#664, where these two layouts are equally good for gemm_strided_batched. But the supertypes of DenseColumnMajor all fall under AbstractIncreasingStrides, while the permuted one has strides (1, 200, 10).

@dlfivefifty
Copy link
Member

Shouldn’t it just be a StridedLayout() since it’s still a strides array?

@mcabbott
Copy link
Contributor Author

mcabbott commented Apr 2, 2020

Yes that's not wrong, it's just that it wouldn't capture the difference between the above and PermutedDimsArray(ones(10,20,30), (3,1,2)), which is (for this purpose) effectively row-major.

I don't know whether such distinctions are common enough to be worth building in here. One could have things like DenseFirstStrided <: StridedLayout, and then the easy case for my function is Union{DenseFirstStrided, DenseColumnMajor}. (Without touching the existing type tree, I think.)

@dlfivefifty
Copy link
Member

Ah it’s There and called ColumnMajor() . Though can you determine this at compile time?

@mcabbott
Copy link
Contributor Author

mcabbott commented Apr 2, 2020

But ColumnMajor <: ArrayLayouts.AbstractIncreasingStrides, which isn't true here.

And yes, the permutation is part of the type of the PermutedDimsArray.

@dlfivefifty
Copy link
Member

Does this need to be at compile time? It seems like you could just dispatch on AbstractStridedLayout and test the strides at run time

@mcabbott
Copy link
Contributor Author

mcabbott commented Apr 2, 2020

Yes that's what my PR does right now, and it does work, but seems a little ugly. (It recursively calls itself & tries to re-arrange strides, and if it can't eventually gives up & calls a fallback function.)

@dlfivefifty
Copy link
Member

Ok, adding DenseFirstStrided makes sense if it keeps the code clean

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 a pull request may close this issue.

2 participants