[libcxx-commits] [PATCH] D56913: decoupling Freestanding atomic<T> from libatomic.a
Olivier Giroux via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Feb 5 21:28:39 PST 2019
__simt__ marked 19 inline comments as done.
__simt__ added a comment.
I will come back with another patch that addresses these comments.
Also will start a thread to debate C++03 support.
================
Comment at: libcxx/include/atomic:651
__gcc_atomic::__can_assign<volatile _Atomic(_Tp)*, _Tp>::value>::type
-__c11_atomic_init(volatile _Atomic(_Tp)* __a, _Tp __val) {
+__cxx_atomic_init(volatile _Atomic(_Tp)* __a, _Tp __val) {
__a->__a_value = __val;
----------------
ldionne wrote:
> jfb wrote:
> > IIRC this `__c11_atomic_*` code is only there under `#if defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)`, which works around limitations when you don't have the compiler builtins? clang otherwise defines them in `include/clang/Basic/Builtins.def`. Have you tried this out on GCC to make sure it works as expected?
> >
> > FWIW the docs are here: https://clang.llvm.org/docs/LanguageExtensions.html#c11-atomic-builtins
> >
> > I think the forwarding layer looks fine, just want to make sure we're on the same page.
> I have the same concern -- I like where this patch is going and TBH I don't think that defining `_Atomic` and `__c11_atomic_xxx` operations in a user-facing way in libc++ is a good idea. But I want to understand what we're breaking by removing that.
Agreed. My code used to live under this backend side, I understand what it does.
But, as you found out just below, I didn't test this version on GCC w/o my primary path active.
================
Comment at: libcxx/include/atomic:896
+template<typename _Tp, typename _Sco>
+struct __cxx_locked_atomic_type {
+
----------------
ldionne wrote:
> __simt__ wrote:
> > ldionne wrote:
> > > Why is this not in `std::`?
> > In the current version it introduces overloads to some symbols that are in the global namespace, and need to be selected against. This being said this version works least-well on Clang and that's probably not good to go. I think the way to address this is the same as your next comment -- reintroduce the interposer layer I had in the first version of the patch.
> Which symbols in the global namespace are you thinking about?
It was `__c11_...` but now it's moot.
================
Comment at: libcxx/include/atomic:929
+static _LIBCPP_INLINE_VISIBILITY inline
+void __c11_atomic_init(volatile __cxx_locked_atomic_type<_Tp, _Sco>* __a, _Tp __val) {
+ __a->__a_value = __val;
----------------
ldionne wrote:
> __simt__ wrote:
> > ldionne wrote:
> > > It seems wrong to have those be named `__c11_atomic_xxx` since they have nothing to do with C11. Instead, the code in `atomic` and `__atomic_base` could make calls to functions called `__atomic_xxx_impl(...)`, and that could be implemented as C11 atomics in one case and as your `__cxx_locked_atomic_type` in another case.
> > >
> > > So basically, instead of introducing this new version of `__c11_atomic_init`, rename the existing one to `__atomic_init_impl`, and then add your version under the name `__atomic_init_impl` too. What do you think? If you agree this would yield more clarity, we could apply this rename in a separate patch prior to this one.
> > >
> > > I don't think the rename would be an ABI break because all those functions are `static inline` today, so they have internal linkage anyway (and hence they exist in each TU where they're used).
> > The code I have here is exactly how this file already works today, it defines `__c11_atomic...` when it's not defined. In my first patch version, I added an interposer layer that laundered the N different backend types through the same `__cxx_atomic...` interface. I will come back with a patch that works that way.
> Well, I was mostly suggesting this approach to see what you (and others) would think of it. It's really the same thing except we change the name from `__c11_atomic_xxx` to `__atomic_xxx_impl` or similar. From your reply I gather that you like this better too.
On Clang it's the only approach that works, AFAICT. GCC was more lenient.
================
Comment at: libcxx/include/atomic:934
+template <typename _Tp, typename _Sco>
+static _LIBCPP_INLINE_VISIBILITY inline
+void __c11_atomic_store(volatile __cxx_locked_atomic_type<_Tp, _Sco>* __a, _Tp __val, int) {
----------------
ldionne wrote:
> You don't need those to be `static inline`. They are templates, so they are implicitly `inline`, and the linkage / visibility is controlled by `_LIBCPP_INLINE_VISIBILITY` (so no need for `static`).
Ok. I'll clean that up. I don't think this header is particularly good about minimizing the decorations.
================
Comment at: libcxx/include/atomic:1045
+template <class _Tp, class _Sco>
+using __atomic_type = typename conditional<__atomic_always_lock_free(sizeof(_Tp), 0),
+ _Atomic_s(_Tp, _Sco),
----------------
__simt__ wrote:
> ldionne wrote:
> > ldionne wrote:
> > > Not as cute, but you could use a traditional metafunction instead of an alias to make this potentially work in C++03. Not that I'm attached to supporting atomics in C++03, but it seems like we do (looking at the tests).
> > Also, I think this layer is interesting because it marks the boundary between implementation and interface. Basically, `atomic` and `__atomic_base` are both used to provide the interface that's mandated by the standard. `__atomic_type` is where the actual implementation begins, be it through `_Atomic` or the `__cxx_locked_atomic_type` you're introducing.
> >
> > I believe that making this explicit in the name of the type might add clarity, for example `__atomic_impl`. WDYT?
> SGTM. I'll use `impl` in the name of the resurrected interposer layer.
We should have a top-level discussion about the need for C++03 support.
================
Comment at: libcxx/include/atomic:557
+ !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) && \
+ !defined(_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP)
#error <atomic> is not implemented
----------------
ldionne wrote:
> IIUC, it doesn't make sense for libc++ to have `_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP` since it's a purely non-libc++ thing. Am I wrong? If not, please remove this knob for now and we can re-add it later on when we have a reason to (e.g. when/if we introduce scoped atomics into libc++).
Actually this has been good for me.
First, there is `_LIBCPP_HAS_THREAD_API_EXTERNAL`, which I've had to set for CUDA in RTC mode. This is analogous to that, in that you don't know what the underlying support is, but something has vouched that it conforms to the contract with the header.
Second, this allows a back-end that isn't GCC or C11 to slide underneath <atomic>. In that case, <atomic> does nothing to help, it assumes outright that the `__cxx_atomic...` symbols exist. That's the contract.
I would like to keep this one, but like anything else, I can keep it in the downstream.
================
Comment at: libcxx/include/atomic:588
+struct __cxx_atomic_scope { };
+#define _LIBCPP_ATOMIC_SCOPE_DEFAULT __cxx_atomic_scope
+#endif
----------------
ldionne wrote:
> Please get rid of scoped atomics for the moment since those are orthogonal to the rest of the patch.
Ok. Next patch won't have them.
================
Comment at: libcxx/include/atomic:658
typename enable_if<
- __gcc_atomic::__can_assign<volatile _Atomic(_Tp)*, _Tp>::value>::type
-__c11_atomic_init(volatile _Atomic(_Tp)* __a, _Tp __val) {
+ __cxx_atomic_type::__can_assign<volatile __cxx_atomic_type<_Tp>*, _Tp>::value>::type
+__cxx_atomic_init(volatile __cxx_atomic_type<_Tp>* __a, _Tp __val) {
----------------
ldionne wrote:
> This rename of `__gcc_atomic::__can_assign` to `__cxx_atomic_type::__can_assign` looks wrong -- `__cxx_atomic_type` is not a namespace. I don't think this will compile. I think that just `__can_assign` is what you need? Same comment applies below.
Whoops. Sorry. Will fix.
================
Comment at: libcxx/include/atomic:1049
+static _LIBCPP_INLINE_VISIBILITY inline
+_Tp* __cxx_atomic_fetch_add(_Atomic(_Tp*) volatile* __a, ptrdiff_t __delta, int __order) {
+ return __c11_atomic_fetch_add(&__a->__a_value, __delta, __order);
----------------
ldionne wrote:
> I think the first parameter should be `__cxx_atomic_type<_Tp*>`, but then that function already exists above.
It exists in another #ifdef, right? I could probably use the `using` name here, if it mattered. This path is the C11 path, though, so `_Atomic` is what we're looking to match.
FWIW, template type deduction involving `_Atomic(_Tp)` is terrible.
================
Comment at: libcxx/include/atomic:1092
+#if defined(_LIBCPP_FREESTANDING) && defined(__cpp_lib_atomic_is_always_lock_free)
+
----------------
ldionne wrote:
> Please implement `_LIBCPP_FREESTANDING` in `__config` as
>
> ```
> #ifndef __STDC_HOSTED__
> # define _LIBCPP_FREESTANDING
> #endif
> ```
>
> This way, `_LIBCPP_FREESTANDING` will be defined whenever `-ffreestanding` is passed to Clang, which is the goal IIUC.
Ok. Will do.
Repository:
rCXX libc++
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56913/new/
https://reviews.llvm.org/D56913
More information about the libcxx-commits
mailing list