[libcxx-commits] [PATCH] D56913: decoupling Freestanding atomic<T> from libatomic.a

[Olivier Giroux via Phabricator via libcxx-commits]

__simt__ marked 5 inline comments as done.
__simt__ added a comment.

Thanks for the comments, Louis, responses below.

I will get working on a new patch with the `__cxx_atomic_..._impl` layer back in. Shouldn't take long, I can mostly revive it from history.



================
Comment at: libcxx/include/atomic:896
+template<typename _Tp, typename _Sco>
+struct __cxx_locked_atomic_type {
+
----------------
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.


================
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:
> 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.


================
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),
----------------
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.


================
Comment at: libcxx/include/atomic:1059
+#ifndef _LIBCPP_ATOMIC_SCOPE_DEFAULT
+#define _LIBCPP_ATOMIC_SCOPE_DEFAULT int
+#endif
----------------
ldionne wrote:
> Can you explain what this scope is?
When it's not short-circuited (like here) its a tag type to dispatch to the right atomic backend for a scoped memory model. i.e. "acquire @device-scope" or "release @system-scope".

This part of the patch is insufficiently-justified, I understand, feel free to ask me more questions.

Here I scoped out (pun intended) the amount of plumbing one would need and it's very little (this line above, and each occurrence of _Sco in the file). You can tell me to rip this out and come back separately but it will affect the non-lock-free adapter in this patch, more significantly than the rest (as you can see).


================
Comment at: libcxx/include/atomic:1265
     _LIBCPP_INLINE_VISIBILITY
-    atomic() _NOEXCEPT _LIBCPP_DEFAULT
+#ifndef _LIBCPP_CXX03_LANG
+    atomic() _NOEXCEPT = default;
----------------
ldionne wrote:
> __simt__ wrote:
> > jfb wrote:
> > > Do we support `atomic` in C++03 mode?
> > I couldn't get this to build fully successfully without -std=c++11.
> > 
> > Here my change is for consistency, this is what's done on other objects in this file.
> I'm confused as to why this is needed. It seems like just `atomic() _NOEXCEPT _LIBCPP_DEFAULT` should still work fine, as even in the case where `_LIBCPP_DEFAULT` expands to `{}`, we get:
> 
> ```
> atomic() _NOEXCEPT {}
> ```
> 
> But that should default-construct `__base` as expected. Do you mean that you tried building in C++03 mode without this change and the tests didn't pass? That sure is surprising. If that's the case, then leave as-is, but otherwise if it's just for consistency then I would stick with what's there.
I haven't actually built this in C++03 mode, though I did get some mileage by piggybacking on this path in CUDA, it's not a good enough reason to keep this in the patch if you don't believe the consistency argument. I can just as well drop this part.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56913/new/

https://reviews.llvm.org/D56913