[libcxx-commits] [PATCH] D103769: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")
Raul Tambre via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jun 9 04:16:40 PDT 2021
tambre added inline comments.
================
Comment at: libcxx/docs/Cxx2aStatusPaperStatus.csv:136
"","","","","",""
-"`P0883 <https://wg21.link/P0883>`__","LWG","Fixing Atomic Initialization","Belfast","* *",""
+"`P0883 <https://wg21.link/P0883>`__","LWG","Fixing Atomic Initialization","Belfast","|Complete|","13.0"
"`P1391 <https://wg21.link/P1391>`__","LWG","Range constructor for std::string_view","Belfast","* *",""
----------------
Quuxplusone wrote:
> tambre wrote:
> > Quuxplusone wrote:
> > > You also need to change the value of `__cpp_lib_atomic_value_initialization` and re-run `libcxx/utils/generate_feature_test_macro_components.py`.
> > >
> > > Also, p0883r2 asks to deprecate `atomic_init`. That could easily be a separate PR, but I don't think we should mark p0883 "Complete" until that's complete.
> > > http://eel.is/c++draft/depr.atomics.general
> > >
> > > (p0883 is also supposed to affect `atomic<shared_ptr<T>>` and `atomic<weak_ptr<T>>`, but libc++ does not implement those specializations (yet). So I think that part is OK.)
> > > You also need to change the value of `__cpp_lib_atomic_value_initialization` and re-run `libcxx/utils/generate_feature_test_macro_components.py`.
> >
> > Added.
> >
> > > Also, p0883r2 asks to deprecate `atomic_init`. That could easily be a separate PR, but I don't think we should mark p0883 "Complete" until that's complete.
> > > http://eel.is/c++draft/depr.atomics.general
> >
> > [[ https://github.com/llvm/llvm-project/commit/06aaf0b3431f29b6debbb96fdd92ada896f336ff | Commit 06aaf0b3431f29b6debbb96fdd92ada896f336ff ]] adjusted the synopsis "to match what is implemented" so I assumed that part was considered done.
> > The required change seems to be simply adding `_LIBCPP_DEPRECATED_IN_CXX20` so I don't think it's worthy of a separate change. I've added it here.
> > The required change seems to be simply adding `_LIBCPP_DEPRECATED_IN_CXX20`
>
> Code change LGTM. We usually want to see a "deprecation warning test" added as well; I think you should add a `libcxx/test/libcxx/depr/depr.atomics.general/atomic_init.depr_in_cxx20.verify.cpp` along the same lines as `libcxx/test/libcxx/depr/depr.function.objects/adaptors.depr_in_cxx11.verify.cpp`. (My impression is that we're not very consistent about these tests, so I suggest copying-and-modifying `adaptors.depr_in_cxx11.verify.cpp` in the obvious way, but if @ldionne asks for some other testing strategy, you should listen to him not me.)
Added note that `shared_ptr` and floating-point changes haven't been applied as they themselves aren't implemented yet.
Added the deprecation test, but in the `std` directory as I don't think this should be a libc++-specific test.
================
Comment at: libcxx/include/atomic:205-206
- atomic() noexcept = default;
+ atomic() noexcept = default; // until C++17
+ constexpr atomic() noexcept; // since C++17
constexpr atomic(T* desr) noexcept;
----------------
Mordante wrote:
> Quuxplusone wrote:
> > tambre wrote:
> > > Quuxplusone wrote:
> > > > Nit: IMHO you could/should make this
> > > > ```
> > > > atomic() noexcept; // constexpr since C++20
> > > > ```
> > > > and likewise below. (I'm guessing this one should say "C++20" like the others, too.)
> > > > But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).
> > > Done.
> > Nit: I think the leading `constexpr` should be dropped:
> > ```
> > atomic() noexcept; // constexpr since C++20
> > ```
> > but if anyone disagrees, I don't care that much. These synopses are all pretty ad-hoc. :)
> > But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).
> I think it would be better to show the difference. (Actually I already wrote a comment about it, only to discover these remarks were a line below.)
>
>
> Nit: I think the leading `constexpr` should be dropped:
I took the leading `constexpr` by example from `<numerics>` synopsis.
But I've now returned this to be separate as it was originally.
================
Comment at: libcxx/include/atomic:205-206
- atomic() noexcept = default;
+ atomic() noexcept = default; // until C++17
+ constexpr atomic() noexcept; // since C++17
constexpr atomic(T* desr) noexcept;
----------------
tambre wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > tambre wrote:
> > > > Quuxplusone wrote:
> > > > > Nit: IMHO you could/should make this
> > > > > ```
> > > > > atomic() noexcept; // constexpr since C++20
> > > > > ```
> > > > > and likewise below. (I'm guessing this one should say "C++20" like the others, too.)
> > > > > But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).
> > > > Done.
> > > Nit: I think the leading `constexpr` should be dropped:
> > > ```
> > > atomic() noexcept; // constexpr since C++20
> > > ```
> > > but if anyone disagrees, I don't care that much. These synopses are all pretty ad-hoc. :)
> > > But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).
> > I think it would be better to show the difference. (Actually I already wrote a comment about it, only to discover these remarks were a line below.)
> >
> >
> > Nit: I think the leading `constexpr` should be dropped:
>
> I took the leading `constexpr` by example from `<numerics>` synopsis.
> But I've now returned this to be separate as it was originally.
> Nit: I think the leading `constexpr` should be dropped:
> ```
> atomic() noexcept; // constexpr since C++20
> ```
> but if anyone disagrees, I don't care that much. These synopses are all pretty ad-hoc. :)
================
Comment at: libcxx/include/atomic:1714
+ _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+ __atomic_base() _NOEXCEPT : __base{} {}
+#else
----------------
Quuxplusone wrote:
> tambre wrote:
> > tambre wrote:
> > > Quuxplusone wrote:
> > > > Personally I'd prefer to see `__base()` here. The Standard says `T()` not `T{}`:
> > > > http://eel.is/c++draft/atomics.types.operations#2
> > > > In fact, don't they have visibly different effects when `T` is `struct A { int i; A() = default; }`? Or is that different in C++20 because aggregate init, and/or different everywhere now due to some DR? I'm having trouble demonstrating the issue on Godbolt: https://godbolt.org/z/o5aY7E3Eb
> > > > I //can// demonstrate that `A a;` and `auto a = A();` have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms
> > > >
> > > > Ditto below (line 1804 and anywhere I missed).
> > > Changed to the parentheses variant.
> > > `T()` and `T{}` are equivalent since they're both variants of value initialization.
> > > `T(args)` and `T{args}` (direct initialization) should be equivalent for aggregates since C++20 as a result of P0960R3 (except in case of narrowing).
> > >
> > > > I //can// demonstrate that `A a;` and `auto a = A();` have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms
> > >
> > > I'm not sure either. P1008R1 did change this to not be aggregate initialization when using `{}`.
> > > Personally I'd prefer to see `__base()` here. The Standard says `T()` not `T{}`:
> > > http://eel.is/c++draft/atomics.types.operations#2
> > > In fact, don't they have visibly different effects when `T` is `struct A { int i; A() = default; }`? Or is that different in C++20 because aggregate init, and/or different everywhere now due to some DR? I'm having trouble demonstrating the issue on Godbolt: https://godbolt.org/z/o5aY7E3Eb
> > > I //can// demonstrate that `A a;` and `auto a = A();` have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms
> > >
> > > Ditto below (line 1804 and anywhere I missed).
> >
> >
> Bah, I missed the first time around that this is initializing `__atomic_base<_Tp, false> __base`, not `_Tp __a_`, anyway! The important parens-not-braces is on line 1679; I now agree it definitely doesn't matter which one we use here. Can't we eliminate this member-initializer entirely, because `__atomic_base<_Tp, false>`'s default ctor (line 1679) will do the right thing? I think the whole `#if` can be removed, and all you need to change here is to add `_LIBCPP_CONSTEXPR_AFTER_CXX17`:
> ```
> _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
> __atomic_base() _NOEXCEPT _LIBCPP_DEFAULT
> ```
> Ditto lines 1800, 1832.
Good idea, done.
================
Comment at: libcxx/include/atomic:2566
+ _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+ atomic_flag() _NOEXCEPT : __a_() {}
+#else
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Since `__a_` is not literally `bool`, I would feel slightly safer if this explicitly said `__a_(false)`; compare line 2573. Sorry for not noticing this the first time around.
> Can you update the test `test/std/atomics/atomics.flag/default.pass.cpp` to validate the new behaviour?
Good catch, the `false` is necessary due to the indirection through `__cxx_atomic_impl`. The rest weren't actually getting properly initialized either. Should be implemented correctly now.
Adjusted the test.
> Can you update the test `test/std/atomics/atomics.flag/default.pass.cpp` to validate the new behaviour?
I'm afraid not, it's already testing the behaviour required by P0883, i.e. that the underlying storage is zeroed on creation.
================
Comment at: libcxx/include/atomic:2566
+ _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+ atomic_flag() _NOEXCEPT : __a_() {}
+#else
----------------
tambre wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Since `__a_` is not literally `bool`, I would feel slightly safer if this explicitly said `__a_(false)`; compare line 2573. Sorry for not noticing this the first time around.
> > Can you update the test `test/std/atomics/atomics.flag/default.pass.cpp` to validate the new behaviour?
> Good catch, the `false` is necessary due to the indirection through `__cxx_atomic_impl`. The rest weren't actually getting properly initialized either. Should be implemented correctly now.
> Adjusted the test.
>
> > Can you update the test `test/std/atomics/atomics.flag/default.pass.cpp` to validate the new behaviour?
> I'm afraid not, it's already testing the behaviour required by P0883, i.e. that the underlying storage is zeroed on creation.
> Can you update the test `test/std/atomics/atomics.flag/default.pass.cpp` to validate the new behaviour?
================
Comment at: libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp:11
// REQUIRES: is-lockfree-runtime-function
+// ADDITIONAL_COMPILE_FLAGS: -Wno-psabi
----------------
Since we're now initializing we otherwise get:
```
In file included from /opt/deb/llvm/libcxx/test/libcxx/atomics/atomics.align/align.pass.pass.cpp:25:
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(int)))) int' (vector of 16 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(int)))) int' (vector of 16 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(int)))) int' (vector of 32 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(int)))) int' (vector of 32 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(float)))) float' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(float)))) float' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(float)))) float' (vector of 32 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(float)))) float' (vector of 32 'float' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(4 * sizeof(double)))) double' (vector of 4 'double' values) without 'avx' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(4 * sizeof(double)))) double' (vector of 4 'double' values) without 'avx' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(double)))) double' (vector of 16 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(double)))) double' (vector of 16 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1681:33: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(double)))) double' (vector of 32 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
__atomic_base() _NOEXCEPT : __a_(_Tp()) {}
^
/opt/deb/llvm/build/include/c++/v1/atomic:1500:7: error: AVX vector argument of type '__attribute__((__vector_size__(32 * sizeof(double)))) double' (vector of 32 'double' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
: _Base(value) {}
^
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103769/new/
https://reviews.llvm.org/D103769
More information about the libcxx-commits
mailing list