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

refactor: remove obsolete cuda_kernels #2876

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

ManasviGoyal
Copy link
Collaborator

@ManasviGoyal ManasviGoyal commented Dec 7, 2023

Removed the following cuda_kernels which had a corresponding cpu-kernels in v1 but not in v2:

  1. awkward_ByteMaskedArray_getitem_carry.cu
  2. awkward_ByteMaskedArray_mask.cu
  3. awkward_Index_to_Index64
  4. awkward_IndexedArray_getitem_nextcarry_outindex_mask
  5. awkward_IndexedArray_mask
  6. awkward_ListOffsetArray_reduce_global_startstop_64
  7. awkward_NumpyArray_contiguous_init
  8. awkward_NumpyArray_contiguous_next
  9. awkward_NumpyArray_fill_tobool
  10. awkward_NumpyArray_getitem_boolean_nonzero
  11. awkward_NumpyArray_getitem_next_array
  12. awkward_NumpyArray_getitem_next_array_advanced
  13. awkward_NumpyArray_getitem_next_at
  14. awkward_NumpyArray_getitem_next_range
  15. awkward_NumpyArray_getitem_next_range_advanced
  16. awkward_carry_arange
  17. awkward_combinations
  18. awkward_content_reduce_zeroparents_64
  19. awkward_index_carry_nocheck
  20. awkward_regularize_arrayslice
  21. awkward_zero_mask

This one was in none of the versions:

  1. awkward_index_carry

@ManasviGoyal ManasviGoyal added the gpu Concerns the GPU implementation (backend = "cuda') label Dec 7, 2023
@ManasviGoyal ManasviGoyal marked this pull request as draft December 7, 2023 12:38
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #2876 (46f7f54) into main (d117334) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
Files Coverage Δ
src/awkward/_connect/cuda/__init__.py 0.00% <ø> (ø)

@ManasviGoyal
Copy link
Collaborator Author

ManasviGoyal commented Dec 7, 2023

@jpivarski @ianna
These are two more that had cpu-kernels in v1 and don't have one in v2

  • awkward_reduce_argmax_bool_64
  • awkward_reduce_argmin_bool_64

I see that they have a different if condition. But since their cpu-kernels were dropped, are they still needed?

@ManasviGoyal ManasviGoyal marked this pull request as ready for review December 7, 2023 14:10
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This is good to do as a first step, so that you don't accidentally spend time on obsolete kennels. The number of removed kennels doesn't surprise me—going from v1 to v2, it became possible to use NumPy in places where we previously needed custom kernels. For the GPU, CuPy takes that role (assuming that CuPy is a good replacement for NumPy in those places, which it very likely is, and if not, a shim in its nplike would fix it).

I'd say you can merge when ready. You only need one review, not two, although if @ianna has anything to say about this, please do!

@ManasviGoyal
Copy link
Collaborator Author

ManasviGoyal commented Dec 7, 2023

This is good to do as a first step, so that you don't accidentally spend time on obsolete kennels. The number of removed kennels doesn't surprise me—going from v1 to v2, it became possible to use NumPy in places where we previously needed custom kernels. For the GPU, CuPy takes that role (assuming that CuPy is a good replacement for NumPy in those places, which it very likely is, and if not, a shim in its nplike would fix it).

@jpivarski These two should be removed too right?

  • awkward_reduce_argmax_bool_64
  • awkward_reduce_argmin_bool_64

@jpivarski
Copy link
Member

If you don't see them in v2, then yes.

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ManasviGoyal - looks good to me! please, merge it when you are done with it. Thanks!

@ManasviGoyal ManasviGoyal force-pushed the ManasviGoyal/remove-obsolete-cuda-kernels branch from eb1f30c to 46f7f54 Compare December 8, 2023 08:33
@ManasviGoyal ManasviGoyal merged commit 432d49b into main Dec 8, 2023
38 checks passed
@ManasviGoyal ManasviGoyal deleted the ManasviGoyal/remove-obsolete-cuda-kernels branch December 8, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Concerns the GPU implementation (backend = "cuda')
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants