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

[Olivier Giroux via Phabricator via libcxx-commits]

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


================
Comment at: include/atomic:1557
+    if(0 != __cxx_atomic_exchange(__cxx_atomic_rebind(&c->credit), (ptrdiff_t)0, memory_order_relaxed)) {
+        __libcpp_mutex_lock(&c->mutex);
+        __libcpp_mutex_unlock(&c->mutex);
----------------
EricWF wrote:
> What's the purpose of the lock and unlock?
This one is fun.

The model for a condition variable is that there is a lock that guards all reads and writes of the variable that has a condition evaluated on it. In this case here, that variable is an atomic, and we both read and write it outside the lock - we evaluate the condition outside the lock with much greater concurrency. However we're still exposed to a race here, because threads may be transitioning into the sleep state as we come to notify.

Taking the lock here resolves this race condition, even if we do nothing with the critical section.


================
Comment at: include/barrier:80
+    atomic<ptrdiff_t>  expected_adjustment;
+    CompletionF        completion;
+
----------------
EricWF wrote:
> Is the completion function ever stateless? If so we'll want to find a way to EBO it.
It can be. How do we do that?


================
Comment at: include/barrier:199
+    alignas(64) atomic<ptrdiff_t> expected, arrived;
+    alignas(64) CompletionF       completion;
+    alignas(64) atomic<bool>      phase;
----------------
EricWF wrote:
> Why is this getting aligned?
This one doesn't need to be. Removing.


================
Comment at: include/barrier:216
+    [[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+    arrival_token arrive(ptrdiff_t update = 1) 
+    {
----------------
EricWF wrote:
> Public declarations and member functions should be implemented in the class that declares them in the standard. 
> 
> Base classes containing implementation details should be inherited privately. 
> 
That's not how it works in <atomic> and I patterned on that. Also it helps the CUDA port be less tedious.

If you feel strongly about this one I can obviously take the extra steps, but do you?


================
Comment at: include/chrono:1065
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
-#ifndef _LIBCPP_CXX03_LANG
+#if !defined(_LIBCPP_CXX03_LANG)
         duration() = default;
----------------
EricWF wrote:
> Is this change intentional?
No.


================
Comment at: include/chrono:1571
 
+#ifndef _LIBCPP_HAS_CLOCK_API_EXTERNAL
+
----------------
EricWF wrote:
> Are these changes intentional?
Yes. CUDA can't make OS calls but we do have another way of implementing this. So I made it external-izable.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480