[libcxx-commits] [PATCH] D68480: Implementation of C++20's P1135R6 for libcxx

[Olivier Giroux via Phabricator via libcxx-commits]

__simt__ marked 7 inline comments as done.
__simt__ added inline comments.


================
Comment at: libcxx/include/atomic:1477
+    if (0 != __cxx_atomic_exchange(&__c->__waiters, (ptrdiff_t)0, memory_order_relaxed))
+        __libcpp_platform_wake((_Tp*)__a, true);
+}
----------------
zoecarver wrote:
> I think this can be a `static_cast`.
Actually it can and should go away now. I'll remove the cast.


================
Comment at: libcxx/include/atomic:1533
+        return;
+    if(0 != __cxx_atomic_exchange(&__c->__credit, (ptrdiff_t)0, memory_order_relaxed)) {
+        __libcpp_mutex_lock(&__c->__mutex);
----------------
zoecarver wrote:
> Might be wrong but, can't this be a CAS (because you are both comparing and swapping `0`)?
It's not conditionally exchanging, it's exchanging unconditionally and then conditionally taking a computation step.


================
Comment at: libcxx/include/atomic:1535
+        __libcpp_mutex_lock(&__c->__mutex);
+        __libcpp_mutex_unlock(&__c->__mutex);
+        __libcpp_condvar_broadcast(&__c->__condvar);
----------------
zoecarver wrote:
> Why do you lock, then immediately unlock the mutex here?
I answered that one here: https://reviews.llvm.org/D68480?id=223282#inline-618652


================
Comment at: libcxx/include/atomic:1563
+            return;
+        if(__i < 12)
+            __libcpp_thread_yield_processor();
----------------
zoecarver wrote:
> Is this another "magic" number? If so, can it be a macro too? 
Sure.


================
Comment at: libcxx/include/atomic:1696
         {return __cxx_atomic_compare_exchange_strong(&__a_, &__e, __d, __m, __m);}
 
+    _LIBCPP_INLINE_VISIBILITY void wait(_Tp __v, memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
----------------
zoecarver wrote:
> This should only be defined after C++17.
That would be unfortunate, I think, <atomic> in general tries to offer its functionality in back versions.

Also, the CUDA port definitely doesn't want to be tied to '17 for quite some time.

Can we leave it on in all dialects that <atomic> supports, like the rest?


================
Comment at: libcxx/include/atomic:1697
 
+    _LIBCPP_INLINE_VISIBILITY void wait(_Tp __v, memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
+        {__cxx_atomic_wait(&__a_, __v, __m);}
----------------
zoecarver wrote:
> You can use `memory_order::seq_cst` here.
Pending other comment's resolution.


================
Comment at: libcxx/include/atomic:1821
     typedef __atomic_base<_Tp> __base;
+    using value_type = _Tp;
     _LIBCPP_INLINE_VISIBILITY
----------------
zoecarver wrote:
> Is this available in C++03? If so, use `typedef`. 
> 
> Actually, do we support any compilers that don't support `using` type aliases?
Will do.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480