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

Mon Feb 4 09:48:53 PST 2019

ldionne added reviewers: mclow.lists, EricWF.
ldionne added inline comments.

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

================
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;
----------------
__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.

================
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) {
----------------
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`).

================
Comment at: libcxx/include/atomic:1059
+#ifndef _LIBCPP_ATOMIC_SCOPE_DEFAULT
+#define _LIBCPP_ATOMIC_SCOPE_DEFAULT int
+#endif
----------------
__simt__ wrote:
> 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).
I think I would do that in a separate patch, this way we can have a longer discussion about whether we want it and whether it's the right way of doing it.

================
Comment at: libcxx/include/atomic:1265
     _LIBCPP_INLINE_VISIBILITY
-    atomic() _NOEXCEPT _LIBCPP_DEFAULT
+#ifndef _LIBCPP_CXX03_LANG
+    atomic() _NOEXCEPT = default;
----------------
__simt__ wrote:
> 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.
It's just that I don't understand why it's needed. If I did, maybe the consistency argument would begin to make more sense. I'll add other reviewers who might know better.

================
Comment at: libcxx/include/atomic:1905
+#ifndef _LIBCPP_ATOMIC_FLAG_TYPE
+#define _LIBCPP_ATOMIC_FLAG_TYPE bool
+#endif
----------------
What's the benefit of macro-izing this? I guess it's because you want to override it under some circumstances?

Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913