This is an archive of the discontinued LLVM Phabricator instance.

Implement C++20 std::atomic_ref and test
Needs ReviewPublic

Authored by BenjaminTrapani on Jan 5 2020, 5:57 PM.

Details

Reviewers
rjmccall
theraven
jfb
mclow.lists
__simt__
jrhemstad
wmaxey
Group Reviewers
Restricted Project
Summary

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?

Diff Detail

Event Timeline

BenjaminTrapani created this revision.Jan 5 2020, 5:57 PM
Herald added a project: Restricted Project. · View Herald Transcript

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.

Remove redundant namespace qualifiers

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.

Any update on this?

griwes added a subscriber: griwes.Jan 28 2020, 9:36 AM

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'
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 5 2020, 1:00 PM
wmaxey added a subscriber: wmaxey.Aug 6 2020, 10:49 AM

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.

ldionne added inline comments.Sep 14 2020, 12:38 PM
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.

rsmith added a subscriber: rsmith.Sep 16 2020, 4:18 PM
rsmith added inline comments.
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).

__simt__ added inline comments.Sep 16 2020, 4:28 PM
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.

BenjaminTrapani marked an inline comment as done.

Pull in original portion of patch

BenjaminTrapani marked 6 inline comments as done.Nov 26 2020, 2:48 PM
BenjaminTrapani added inline comments.
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...

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 6:59 PM