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

Enable negative indexing for cuda atomic operations #6695

Merged
merged 3 commits into from Feb 19, 2021

Conversation

ashutoshvarma
Copy link
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.

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 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
Contributor Author

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

@gmarkall
Copy link
Member

gmarkall commented Feb 9, 2021

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
Copy link
Member

esc commented Feb 9, 2021

/azp run

@azure-pipelines
Copy link
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
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 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
Copy link
Member

esc commented Feb 9, 2021

@gmarkall seems like AZP fine this time around.

@esc
Copy link
Member

esc commented Feb 9, 2021

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
Copy link
Member

esc commented Feb 9, 2021

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
Copy link
Member

gmarkall commented Feb 9, 2021

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

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

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.

None yet

5 participants