Page MenuHomePhabricator

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
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?

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