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

[Olivier Giroux via Phabricator via libcxx-commits]

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


================
Comment at: include/atomic:637
+bool __cxx_atomic_compare_equal(_Tp const& __lhs, _Tp const& __rhs) {
+    return memcmp(&__lhs, &__rhs, sizeof(_Tp)) == 0;
+}
----------------
EricWF wrote:
> This doesn't seem like an implementation of an atomic comparison. 
Right, but I needed a comparison equivalent to CAS for some user-defined types. Admittedly I'm not 100% sure that the user-defined types in the atomic tests aren't the element to blame here, maybe for not having trivial comparison that works.


================
Comment at: include/atomic:1464
+template<typename _Tp = int>
+__cxx_atomic_impl<_Tp>* __cxx_atomic_rebind(_Tp* __inst) {
+    static_assert(sizeof(__cxx_atomic_impl<_Tp>) == sizeof(_Tp),"");
----------------
EricWF wrote:
> I'm not sure I'm comfortable with this atomic type-punning.
That's fair.

Though, two things:
1. This header needs a major overhaul to support atomic_ref. This is just dipping toes in before the overhaul makes this actually legit. But it's going to be a big patch in its own right.
2. The variables in the contention structure introduced in __threading_support need to be accessed atomically, but it has to be declared before the atomic types in <atomic>. Luckily we do control their type and using integers gives us some hope that the underlying __atomic operations will work.


================
Comment at: include/atomic:2791
 
+static_assert(ATOMIC_INT_LOCK_FREE, "This library assumes atomic<int> is lock-free.");
+
----------------
EricWF wrote:
> I'm pretty sure we're still required to compile even if `int` doesn't have lock-free atomic operations.
> 
> Shouldn't we just omit the typedefs?
The Standard, yes, but libcxx itself? What platform actually (not theoretically) is supported by libcxx and does not have lock-free "int"?


================
Comment at: include/barrier:46
+
+#ifndef __simt__
+#include <__config>
----------------
EricWF wrote:
> What's supposed to happen if `__simt__` is defined?
Oh, oops, that's a leftover from the CUDA port. Never mind that.


================
Comment at: include/barrier:67
+
+struct EmptyCompletionF 
+{
----------------
EricWF wrote:
> Reserved name?
Yeah.


================
Comment at: include/barrier:91
+    };
+    ::std::vector<__state_t>   state;
+
----------------
EricWF wrote:
> `vector` should resolve to the correct name. No need to qualify it.
Also a leftover from CUDA port.


================
Comment at: include/barrier:94
+    inline _LIBCPP_INLINE_VISIBILITY
+    bool __arrive(__phase_t const old_phase)
+    {
----------------
EricWF wrote:
> `old_phase`, `half`, and friends all need to be reserved names.
Yes to all these, but let's focus on the design for now.


================
Comment at: include/barrier:105
+        for(size_t round = 0;; ++round) {
+            assert(round <= 63);
+            if(current_expected == 1)
----------------
EricWF wrote:
> The library doesn't use `assert` internally.
Didn't know that. Will strip those.


================
Comment at: include/cstddef:63
 #if _LIBCPP_STD_VER > 14
+#ifdef _LIBCPP_BEGIN_NAMESPACE_STD_NOVERSION
+_LIBCPP_BEGIN_NAMESPACE_STD_NOVERSION
----------------
EricWF wrote:
> I don't think these changes are meant to be here.
I think they are. Every naked "namespace std" is a thing that prevents us from replacing the namespace name. Libcxx is pretty uneven in applying the namespace macros and I would prefer if it always did it.


================
Comment at: include/latch:60
+{
+    alignas(64) atomic<ptrdiff_t> counter;
+public:
----------------
EricWF wrote:
> What's with all the explicit alignment specifications?
It's pretty crucial to performance that synchronization objects (when they are implemented with atomics) not share cache lines. In theory this would use hardware_destructive_interference_size but it remains unimplemented. Meanwhile, as luck would have it, the magic number 64 is right for ~everyone.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480