-
Notifications
You must be signed in to change notification settings - Fork 9
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
Implement array reshape for CUDA #47
base: main
Are you sure you want to change the base?
Conversation
/ok to test |
I've just re-targeted this to |
I merged main into this to test it on Windows; it's failed there due to a mismatching prototype:
However, I don't know if this is a Windows issue or an issue arising from the merge, or me resolving conflicts incorrectly. To try and work it out, I've pushed up the merged version to see what happens in CI here. |
/ok to test |
Looks like this has a Windows-related issue. I'll fix style so I don't leave this PR in a worse state than when I encountered it, and see if I can work out what's going wrong on Windows. |
/ok to test |
On Windows, the implementation of the function has the following signature: .visible .func (.param .b32 func_retval0) numba_attempt_nocopy_reshape(
.param .b32 numba_attempt_nocopy_reshape_param_0,
.param .b64 numba_attempt_nocopy_reshape_param_1,
.param .b64 numba_attempt_nocopy_reshape_param_2,
.param .b32 numba_attempt_nocopy_reshape_param_3,
.param .b64 numba_attempt_nocopy_reshape_param_4,
.param .b64 numba_attempt_nocopy_reshape_param_5,
.param .b32 numba_attempt_nocopy_reshape_param_6,
.param .b32 numba_attempt_nocopy_reshape_param_7
) However, the Numba-generated code declares it as: .extern .func (.param .b32 func_retval0) numba_attempt_nocopy_reshape
(
.param .b64 numba_attempt_nocopy_reshape_param_0,
.param .b64 numba_attempt_nocopy_reshape_param_1,
.param .b64 numba_attempt_nocopy_reshape_param_2,
.param .b64 numba_attempt_nocopy_reshape_param_3,
.param .b64 numba_attempt_nocopy_reshape_param_4,
.param .b64 numba_attempt_nocopy_reshape_param_5,
.param .b64 numba_attempt_nocopy_reshape_param_6,
.param .b32 numba_attempt_nocopy_reshape_param_7
) There are some mismatches in the size of parameters, where they are 32-bit in the implementation but 64-bit from Numba's perspective. |
*/ | ||
#define NPY_MAXDIMS 32 | ||
|
||
typedef long int npy_intp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that this should be something other than a long int
on Windows.
This is 64 bits on Linux and Windows, which corrects an issue with the prototypes not matching on Windows.
/ok to test |
Using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tests are fine, but they should be in test_array.py
rather than test_cuda_array_interface.py
. test_cuda_array_interface.py
is for testing the implementation of the CUDA Array Interface.
@@ -430,6 +455,55 @@ def f(x, y): | |||
# Ensure that synchronize was not called | |||
mock_sync.assert_not_called() | |||
|
|||
# @skip_unless_cuda_python('NVIDIA Binding needed for NVRTC') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When moving the tests, I think this comment can be removed, as it pertained to an old version of Numba.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of small suggestions on moving the tests.
I ported numba/numba#8458 to this repo. The original PR is quite complete.
link_to_library_functions
.Anything else need to be tackled?