-
Notifications
You must be signed in to change notification settings - Fork 68
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
adding atomix tests #308
base: main
Are you sure you want to change the base?
adding atomix tests #308
Conversation
#306 (comment) <--- continuing this discussion with @tkf here as it is related to getting Atomix support in KA So my understanding is that for the short-term in KA, we need to use AtomixCUDA for CUDA operations and a newer version of UnsafeAtomics for CPU? My assumption is that this will fix the two big errors here, right? From there, I guess it's just a matter of figuring out how to write certain tests in Atomix's non-macro interface (and macro interface, I guess). |
What are the big errors? "Known issues that need to be fixed:" in the OP?
Please see what are tested in https://github.com/JuliaConcurrent/UnsafeAtomics.jl/blob/934c34df1fd0fac0b6fa6edc07d98a05b9f98395/test/UnsafeAtomicsTests/src/test_core.jl#L9-L55. If what you want to use are not tested there, I don't think it's useful to test them here.
AtomixCUDA.jl simply calls However, note that AtomixCUDA.jl completely ignores atomic ordering. Maybe the least bad approach is to dispatch to Also, similar to the comments on the atomic ops on CPU, I think it'd be better to test that they are CI'ed on GPU first. Note that I have test working locally but I'm still waiting for buildkite to be enabled (or rather, I haven't been pinging the GPU guys enough to let them enable the build server) In general, nothing special should be done in KA since atomic ops are implemented at much lower level. I feel it would be more constructive to test only that the "KA magics" do not interact badly with the atomics implementations (e.g., maybe |
Hey @tkf. You did not answer my questions in the previous post, I believe I may have poorly communicated my intentions here, so let me be more explicit: Things we both agree on:
Now for the questions: How do I get
This raises a bunch more questions than it solves. Namely:
How do I get += working on floats? What do I do for the specific tests mentioned above of inc, dec, and, or, and xor? As direct answers, I believe it's just a What does "I think it'd be better to test that they are CI'ed on GPU first" mean? Now for some comments: On tests
Initially, I agreed with this statement, and was going to say that these are tests that should be in Atomix. I have personally wasted literal months of time trying to hunt down bugs in KA that would have been caught if we just had a test for API changes upstream. More than that, the tests here are a way to show users, "Here is how you can do atomic addition. Here is how to do atomic max..." There should also be docs when these tests pass with pretty much the same functionality and showing the difference between what we currently have and CUDA. I believe this is essential for KAs wider adoption The state of Atomix Overall, I am not attacking you. I am trying to fix everything that is currently broken in #299. I am also really running out of time. I have already spent months on this and know I still need to add docs once these tests pass. |
Ok, after calling @tkf, I think we have solutions to most things:
I'll be working on what I can in the next few weeks when I have time, but since we have Atomix in KA working for most things and #306 for back-up, this PR is on a slightly lower priority. |
This is a separate PR for the atomix tests from #306. Still needed tests:
Known issues that need to be fixed: