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

Another attempt at supporting non-contiguous arrays #171

Closed
wants to merge 25 commits into from

Conversation

SyamGadde
Copy link
Contributor

@SyamGadde SyamGadde commented Feb 21, 2018

NOTE: SUPERCEDED BY #172.

Inspired by:
https://lists.tiker.net/pipermail/pycuda/2016-December/004986.html
https://gitlab.tiker.net/inducer/pycuda/merge_requests/1

I tried a new approach to supporting non-contiguous arrays in PyCUDA (could be ported to PyOpenCL somewhat easily I think). The goals (some elicited by the above discussion and comments in the WIP) were:

  • element-wise support for arbitrarily-strided arrays (including negative strides)
  • backwards compatibility with current uses of get_elwise_module and get_elwise_range_module
  • limited performance impact on contiguous arrays
  • avoid use of '*', '%', and '/' in calculating indices, at least within the element-wise loop

The only way I could think to support all those goals was to delay compilation (and source generation) to call-time, to take advantage of knowledge of input array strides. Contiguous arrays get the kernels that PyCUDA has always given them, non-contiguous arrays get specialized kernels. The nice thing about doing this is that the actual shape and strides can be sent as '#define's to aid compiler optimization (could even help with the contiguous kernels, though have not tried that). The tricky thing about doing this is that some functions in the current implementation require the Module/Function before call-time, to get texrefs etc. So I basically implemented a Proxy class for SourceModule, called DeferredSourceModule which also defers the generation of the values created by get_function(), get_texref(), etc. until call-time.

To make this all work, indexing (for non-contiguous arrays at least) for an array A needs to be A[A_i], rather than A[i]. If it detects matching contiguous arrays as inputs, then X_i is '#define'd to be i, so kernels using the old method will still work (as long as the input arrays are contiguous and match in strides). No regexes needed to transform the user-specified kernel fragments, it's all directed by the user. Also, if you want to support non-contiguous arrays, you need to send the actual GPUArray objects, rather than their gpudata members to the call or prepare_ functions.

All existing tests succeed. More would probably need to be added if it made sense to integrate this into PyCUDA.

Positive side-effects: GPUArray.get(), GPUArray.set() and GPUArray.copy() now work for arbitrarily sliced/strided arrays.

The performance hit for contiguous arrays is around 15% for modest-sized arrays (i.e. the 1000x100 array tested by Keegan in the above discussion) and, looking at profiling output, I think the hit is due to detecting contiguity/order (in ElementwiseSourceModule.create_key()). This could probably be improved. Performance for non-contiguous arrays is infinitely better, given that they weren't supported before, but I've seen a 40% slowdown over the contiguous version for the b1[::2,:-1]**2 test Keegan tried, due to the need to calculate indexes at each iteration of the loop. It tries to do this in a smart way, by pre-calculating the per-thread step for each dimension, and only using division/modulo to calculate the starting indices for each thread before the loop.

Independently of whether these changes are merged in, I will continue to use and develop them to support some local needs, so comments are welcome. I hope this is useful!

@SyamGadde
Copy link
Contributor Author

SyamGadde commented Feb 21, 2018

Also, wanted to reference the issues this patch may address:

#15
#66
#121
#145
#151
#154
#162

@inducer
Copy link
Owner

inducer commented Feb 22, 2018

Wow. This looks like an impressive bit of work. I agree with the objectives here, and I would like to see this (or something like it) get merged. Unfortunately, I won't be able to take a close look before mid-April (work deadlines). Sorry about the long delay. I've made a note to take a look then. In the meantime, I think it would be a good idea to solicit reviews from folks on the mailing list.

Most of all, thanks for working on this!

@SyamGadde
Copy link
Contributor Author

Absolutely. I will subscribe to the list and post. Thanks!

@SyamGadde
Copy link
Contributor Author

Closing and creating a new pull request #172 with more fixes, more reasonably arranged.

@SyamGadde SyamGadde closed this Mar 1, 2018
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.

4 participants