Skip to content

feat: add some simpler cuda_kernels#2880

Merged
ManasviGoyal merged 11 commits into
mainfrom
ManasviGoyal/add-simple-cuda-kernels
Dec 20, 2023
Merged

feat: add some simpler cuda_kernels#2880
ManasviGoyal merged 11 commits into
mainfrom
ManasviGoyal/add-simple-cuda-kernels

Conversation

@ManasviGoyal

@ManasviGoyal ManasviGoyal commented Dec 8, 2023

Copy link
Copy Markdown
Collaborator

Added some cuda_kernels

  • awkward_ByteMaskedArray_reduce_next_nonlocal_nextshifts_fromshifts_64
  • awkward_IndexedArray_fill
  • awkward_ListArray_fill
  • awkward_IndexedArray_numnull_unique
  • awkward_RegularArray_rpad_and_clip_axis1
  • awkward_index_rpad_and_clip_axis0
  • awkward_UnionArray_simplify
  • awkward_UnionArray_simplify_one
  • awkward_UnionArray_fillindex
  • awkward_UnionArray_fillindex_count
  • awkward_UnionArray_filltags
  • awkward_UnionArray_filltags_const
  • awkward_NumpyArray_fill

@ManasviGoyal ManasviGoyal added the gpu Concerns the GPU implementation (backend = "cuda') label Dec 8, 2023
@ManasviGoyal
ManasviGoyal marked this pull request as draft December 8, 2023 12:38
@ManasviGoyal
ManasviGoyal deleted the ManasviGoyal/add-simple-cuda-kernels branch December 8, 2023 12:41
@ManasviGoyal
ManasviGoyal restored the ManasviGoyal/add-simple-cuda-kernels branch December 8, 2023 15:13
@ManasviGoyal ManasviGoyal reopened this Dec 8, 2023
@ManasviGoyal
ManasviGoyal force-pushed the ManasviGoyal/add-simple-cuda-kernels branch from d4f28c4 to 2e893dd Compare December 8, 2023 15:14
@ManasviGoyal
ManasviGoyal force-pushed the ManasviGoyal/add-simple-cuda-kernels branch from c791e2c to 3ff1e2e Compare December 11, 2023 08:43
@codecov

codecov Bot commented Dec 11, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1cc22e5) 81.98% compared to head (83cafc9) 81.98%.

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

@ManasviGoyal
ManasviGoyal marked this pull request as ready for review December 12, 2023 08:39

@jpivarski jpivarski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These look good, but they need tests (user-level tests in tests-cuda).

Do you know which ak.* functions would test them? One way to find out is to search the codebase for a kernel name and put in an exception before the kernel runs, then run everything in the tests directory to see what fails on the CPU, then just copy that test into tests-cuda. If you have a choice, you'd want to find one that doesn't trigger other kernels that don't yet exist in CUDA.

Another way to search is to do the same in the tests-cuda directory with an exception on one of the kernels that's similar to the kernel whose test you want to add. The tests in the tests-cuda directory are already designed around not touching kernels that don't yet exist. (Eventually, everything will need to work, but you want to get there in small steps.)

These can be merged and their tests can come in another PR, or you can put the tests in this PR for better bookkeeping (so that you can be sure that you got them all; the diff makes a good checklist).

@ianna ianna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ManasviGoyal and @jpivarski - I'm ok with the one-to-one copy of the CPU to GPU kernels. However, our code readability has improved a lot (thanks to @agoose77 and @ioanaif :-) I was wandering if we should make the kernels code more readable (e.g self-documented) by using similar to the code base variable names?

Comment thread src/awkward/_connect/cuda/cuda_kernels/awkward_ListArray_fill.cu
Comment thread src/awkward/_connect/cuda/cuda_kernels/awkward_ListArray_fill.cu
@jpivarski

Copy link
Copy Markdown
Member

I was wandering if we should make the kernels code more readable (e.g self-documented) by using similar to the code base variable names?

I'd prefer to not refactor the kernel names and the names of their arguments because

  • they're buried deeply, call it "L5" in our public-private scheme
  • refactoring will take time and probably introduce some bugs
  • right now, there's a big job to do—adding the new kernels—and I don't believe that this job would be easier if the kernel names and arguments were more in line with how they're used in the codebase. The problem of writing kernels is almost perfectly factored out of how the kernels are used in the codebase. "Almost" refers to the fact that the choice of good test cases would come from how they're intended to be used. (In fact, that criterion was already used to define most of the unit-tests in the kernel-test-data.json.)

@jpivarski

Copy link
Copy Markdown
Member

This PR doesn't affect awkward-cpp, so it does not need to go before @agoose77's upcoming release of awkward-cpp and awkward.

@ianna ianna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No objections to merge it :-)

@ManasviGoyal
ManasviGoyal merged commit b94df54 into main Dec 20, 2023
@ManasviGoyal
ManasviGoyal deleted the ManasviGoyal/add-simple-cuda-kernels branch December 20, 2023 08:20
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.

4 participants