Skip to content

Enable negative indexing for cuda atomic operations - #6695

Merged
sklam merged 3 commits into
numba:masterfrom
ashutoshvarma:cuda-atomic-neg-idx
Feb 19, 2021
Merged

sklam merged 3 commits into
numba:masterfrom
ashutoshvarma:cuda-atomic-neg-idx

Conversation

@ashutoshvarma

Copy link
Copy Markdown
Contributor

Ref #6496

As titled, enables wraparound in CUDA atomic operations.

Instead of adding new tests, I modified existing add atomics tests test_atomic_add_* to prevent unnecessary redundancy in test code.
If the separate test is a must then I can add that too.

@gmarkall gmarkall 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.

Many thanks for the PR! I think this is looking great in general, with just one suggestion / question:

  • Tests pass for me locally with hardware (Quadro RTX 8000), the simulator, and under cuda-memcheck with no errors.
  • The testing additions look good - I see that testing with negative indexing is only added for a subset of test cases, which I think covers the new functionality sufficiently without bloating the set of tests inordinately (as opposed to adding negative indexing to every single test).
  • I'm happy with the additions being made to existing test cases, as it enables reuse of their code instead of either duplicating the rest of the test body or adding another level of indirection in the testing.
  • The style of the changes looks good / seems to be in keeping with the surrounding code.

One thing I'd like to see - either I've missed it or it's not present - is testing of wraparound on arrays in global memory. In the test changes, I can only see tests for negative indexing on shared memory - if negative indexing on global memory is tested here, can you indicate where that testing is please? If not, would it be possible to add testing on negative indexing on global memory in a similar fashion to the testing on shared memory?

Many thanks again!

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Feb 8, 2021
@ashutoshvarma

Copy link
Copy Markdown
Contributor Author

My bad, seems I missed global memory tests. I will update them in a minute.

@gmarkall

gmarkall commented Feb 9, 2021

Copy link
Copy Markdown
Member

Many thanks for the update!

The test fail is just a conda error - could this have a re-run on Azure Pipelines please? (@esc / @stuartarchibald)

@esc

esc commented Feb 9, 2021

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Contributor
Azure Pipelines successfully started running 1 pipeline(s).

@gmarkall gmarkall added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on author Waiting for author to respond to review labels Feb 9, 2021

@gmarkall gmarkall 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.

Many thanks for adding the tests with global memory - they run fine for me locally with hardware, the simulator and cuda-memcheck. This now looks good to me!

@esc could this have a CUDA buildfarm run please?

@esc

esc commented Feb 9, 2021

Copy link
Copy Markdown
Member

@gmarkall seems like AZP fine this time around.

@esc

esc commented Feb 9, 2021

Copy link
Copy Markdown
Member

Running BF: numba_smoketest_cuda_yaml_11

@gmarkall gmarkall added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Feb 9, 2021
@esc esc 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 labels Feb 9, 2021
@esc

esc commented Feb 9, 2021

Copy link
Copy Markdown
Member

BF passed on this.

@gmarkall gmarkall removed the 4 - Waiting on CI Review etc done, waiting for CI to finish label Feb 9, 2021
@gmarkall

gmarkall commented Feb 9, 2021

Copy link
Copy Markdown
Member

@stuartarchibald @sklam would you like to take a look at this before it goes RTM?

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch @ashutoshvarma looks good, thanks for reviewing @gmarkall

@stuartarchibald stuartarchibald added the 5 - Ready to merge Review and testing done, is ready to merge label Feb 19, 2021
@sklam
sklam merged commit b5e449d into numba:master Feb 19, 2021
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.

5 participants