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

CUDA: Add binary ufunc support #8974

Merged
merged 3 commits into from Jun 21, 2023
Merged

Conversation

Matt711
Copy link
Contributor

@Matt711 Matt711 commented May 23, 2023

Following from #8294, this PR adds support for binary ufuncs for the CUDA target (bitwise_and, bitwise_or, etc.). Currently there aren't any unit tests for np.left_shift and np.right_shift. I didn't add them because there was a comment here saying the tests were not added for the CPU target either.

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

From a first pass over this, I think this is looking good like the pattern for implementing ufuncs in CUDA is followed correctly - I have a suggestion and a comment on the diff for now, and will have another pass once I understand the responses.

numba/cuda/tests/cudapy/test_ufuncs.py Show resolved Hide resolved
@@ -272,4 +272,55 @@ def np_real_atanh_impl(context, builder, sig, args):

db[np.degrees] = db[np.rad2deg]

db[np.bitwise_and] = {
'll->l': numbers.int_and_impl,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we not have implementations for unsigned types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I added the missing types from np.<ufunc>.types except O which is a generic python object

@gmarkall
Copy link
Member

gpuci run tests

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review CUDA CUDA related issue/PR labels May 23, 2023
@gmarkall gmarkall added this to the Numba 0.58 RC milestone May 23, 2023
@Matt711 Matt711 requested a review from gmarkall May 24, 2023 13:16
@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels May 24, 2023
@gmarkall
Copy link
Member

gpuci run tests

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Many thanks for the updates! A couple of quick questions / observations on the diff.

numba/cuda/ufuncs.py Show resolved Hide resolved
Comment on lines 330 to 331
db[np.bitwise_not] = db[np.invert]

Copy link
Member

Choose a reason for hiding this comment

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

I think this is superflous, and just reassigns the same key in the dict - these two keys are the same:

$ python -c "import numpy as np; print(np.invert is np.bitwise_not)"
True

I think this can be removed.

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels May 24, 2023
@gmarkall
Copy link
Member

gpuci run tests

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels May 24, 2023
@Matt711 Matt711 requested a review from gmarkall May 24, 2023 20:43
@gmarkall gmarkall added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels May 24, 2023
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Many thanks for the updates! This looks good now.

I think I'd like a buildfarm run for this, since it's the first set of new ufuncs we've added since the addition of the feature - maybe eventually these additions will seem "safe" / generally expected to work everywhere if they work in gpuCI, but I'm not confident there yet.

@stuartarchibald / @sklam Could this have a buildfarm run please?

@gmarkall
Copy link
Member

I understand from an OOB message that numba_smoketest_cuda_yaml_205 ran and passed on this PR.

@gmarkall gmarkall added BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jun 21, 2023
@gmarkall gmarkall added the 5 - Ready to merge Review and testing done, is ready to merge label Jun 21, 2023
@sklam sklam merged commit ebb5c01 into numba:main Jun 21, 2023
23 checks passed
@gmarkall gmarkall mentioned this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed CUDA CUDA related issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants