Implement the atomic ref class by reusing the atomic_base_impl class and friends. Add overloads as required for reference types. The entire spec is tested and working under clang and g++. I could not figure out how to test the implementation falling back to locks (only used in freestanding builds). The additional code there needs to be tested. Should I investigate adding a regression test for freestanding atomic builds?
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
That's a creative use of __cxx_atomic_impl that I hadn't foreseen. Seems to work. Ok.
It would be good if we could de-duplicate code here, because we're on a trajectory of having 4+ versions of every function that differ only on the type they take. Sounds like a perfect opportunity to sprinkle some enable_if here with a helper trait to determine if the thing is a cxx_atomic_impl, volatile cxx_atomic_impl, cxx_atomic_impl of reference, or volatile cxx_atomic_impl of reference.
As for the freestanding mode (or to be more accurate, the builtin-only mode) the only option you have for atomic_ref is to static_assert that it is always lock-free. There is presently 1 platform that builds with this mode (CUDA 10.2+) and this is what we planned to do anyway.
Get rid of duplication across __cxx_atomic_* functions by using a traits class and SFINAE to check types and select overrides when required. Static assert that template type is always lock free when built using builtin only mode.
Provide only two versions of __cxx_atomic_pointer_to_data that perform different cast operations. Get rid of type-specific versions by extracting original type qualifiers and re-annoting atomic type.
For those that are interested in testing out this change, here is a release build of clang 10 with these changes: https://skytopsoftware.ddns.net/binaries/compilers/llvm/llvm_10_release_linux_amd64.tar.gz
Changes are built as of d32170dbd5b0d54436537b6b75beaf44324e0c28 with the following:
cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/home/benjamintrapani/libs/llvm-10-release -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;compiler-rt;debuginfo-tests;libc;libclc;libcxx;libcxxabi;libunwind;lld;lldb;mlir;openmp;parallel-libs;polly;pstl'
Thanks for working on that! I left some general comments, however I believe the best person to give you an in-depth review about this is @__simt__, as a co-author of the paper and as being the de-facto owner of <atomic>.
Misc note: Please update libcxx/www/cxx2a_status.html.
libcxx/include/atomic | ||
---|---|---|
112 | // since C++20 (applies below too) | |
130 | It is customary to put these members closer to the top. | |
847 | Reserved identifiers are of the form _Foo, not Foo_. You'll need to make that (simple) change throughout. | |
1464 | What is preventing you from adding this check on Clang today? | |
1810 | You should guard this with #if _LIBCPP_STD_VER > 17 (applies below too). | |
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp | ||
138 | I believe it would be better to add new test files just for atomic_ref. | |
libcxx/test/std/atomics/atomics.types.generic/trivially_copyable_ref.fail.cpp | ||
55 | How do you expect it to fail? I would strongly suggest using a .verify.cpp test instead and pinning down the error you're expecting to see. |
We're looking at this patch in the context of the libcudacxx fork. I don't have more feedback right now but we probably will get there soon.
libcxx/include/atomic | ||
---|---|---|
1810 | We will want to allow using this from C++11 onwards. If libcxx doesn't do that out of the box, then we'll be doing that in our private patchset. | |
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp | ||
138 | Actually, I would prefer if we found a way to make the atomics tests cover atomic_ref, at breadth. There is absolutely no way that this patch has sufficient test coverage right now, and duplicating all the tests is an inefficient use of our time (now and in the future). For the most part, this could be tested by doing sufficiently clever refactoring in atomic_helpers. |
libcxx/include/atomic | ||
---|---|---|
1810 | Ok. I guess we've been down that road already anyway. | |
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp | ||
138 | As you prefer. I imagine it might look like some generic test that works with atomic *and* atomic_ref. If so, it's easy to have the generic test in a header file and then instantiate it once from a atomic.pass.cpp file and once from a atomic_ref.pass.cpp. This would keep the test structure consistent to what it is today. But I am not strongly attached to a particular approach -- whatever's more convenient works, I was just trying to retain some consistency with the way we've been doing things. |
libcxx/include/atomic | ||
---|---|---|
990 | I find this cast concerning. There's no guarantee that _Atomic(T) is compatible with plain T in general (they might have different representations), and even if the layout is identical, I don't think Clang's strict aliasing rules provide a guarantee that you can access a T via an _Atomic(T). (We happen to treat atomic types as can-alias-anything right now, but that's only because we've not implemented strict aliasing for atomics yet.) Can we use the implementation in terms of __atomic_* for atomic_ref instead? Those builtins are designed to be used on objects that are declared with type T rather than type _Atomic(T). |
libcxx/include/atomic | ||
---|---|---|
990 | I agree with you there. It looks like atomic_ref<> is not implementable (at least not at high QoI) on top of C11 the way that atomic<> was. I do wish the _Atomic(T) path simply wasn't there in libcxx, and we just always used the __atomic_* path but the Mac build chooses the _Atomic(T) path. At least it does today. Not sure how to reconcile that. |
libcxx/include/atomic | ||
---|---|---|
990 | Sorry for the delay in responding to all these comments. I will fix per these comments today. For the purposes of this change, is it ok to leave atomic_ref unimplemented for the C11 path, and look into removing the C11 path later? |
Remove C11 atomic path from atomic header. Update tests to check for specific errors. Other style fixes.
libcxx/include/atomic | ||
---|---|---|
1464 | Should be fine to enable, although probably best done in another patch so we can revert if it breaks many things for a large clang user. | |
libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp | ||
138 | I thought about this a bit and the interfaces under test are substantially different. We could share the common parts by always constructing from lvalues. |
It looks like this went silent. I'm interested in atomic_ref as a downstream user; @BenjaminTrapani are you still interested in pushing this forward? If not I might be able to find someone to look into it...
// since C++20 (applies below too)