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

Olivier Giroux via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Oct 8 15:45:30 PDT 2019


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

I've processed a number of issues for my next patch.



================
Comment at: include/__threading_support:522
+    for(int count = 0;;) {
+      if(__builtin_expect(__f(),1))
+        return true;
----------------
zoecarver wrote:
> Should we put some ifdefs around `__builtin_expect`?
I'll just get rid of these for this patch.


================
Comment at: include/__threading_support:566
+
+using __libcpp_platform_wait_t = int;
+
----------------
__simt__ wrote:
> zoecarver wrote:
> > If this file is used before C++11, this should be a typedef.
> I think this should be C++11 or later, will need an ifdef here.
Fixed in next patch.


================
Comment at: include/atomic:633
 
+extern "C" int memcmp( const void * ptr1, const void * ptr2, size_t num );
+
----------------
__simt__ wrote:
> zoecarver wrote:
> > Why can't you just use `std::memcmp`?
> I'd like to but I didn't want to pull the headers that define it into this one. 
In the next patch I'll just use std::memcmp instead.


================
Comment at: include/atomic:637
+bool __cxx_atomic_compare_equal(_Tp const& __lhs, _Tp const& __rhs) {
+    return memcmp(&__lhs, &__rhs, sizeof(_Tp)) == 0;
+}
----------------
zoecarver wrote:
> __simt__ wrote:
> > 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.
> I think there's an atomic builtin that would work for this. 
No. But in the next version I just renamed this to "nonatomic" compare. Still, I'm not 100% sure this isn't a test problem. Should vet that in the future.


================
Comment at: include/atomic:1504
+    auto const __version = __cxx_atomic_load(__cxx_atomic_rebind(&__c->__version), memory_order_relaxed);
+    if (__builtin_expect(!__cxx_atomic_compare_equal(__cxx_atomic_load(__a, __order), __val), 1))
+        return;
----------------
zoecarver wrote:
> Are you sure `__builtin_expect` is helping here? If so, it may need to be wrapped in `#ifdef`s. 
Removed.


================
Comment at: include/atomic:1533
+    auto * const __c = __libcpp_contention_state(__a);
+    __cxx_atomic_thread_fence(memory_order_seq_cst);
+    if (0 != __cxx_atomic_load(__cxx_atomic_rebind(&__c->__waiters), memory_order_relaxed))
----------------
zoecarver wrote:
> `memory_order_seq_cst`  is depricated, use `memory_order::seq_cst`.
That would be nice but we can't do that in this file if we want to keep building in '11 mode.


================
Comment at: include/atomic:1536
+#endif
+        __libcpp_platform_wake((_Tp*)__a, true);
+}
----------------
zoecarver wrote:
> Is this c-style cast OK? 
It's the mirror image of the one that Eric pointed out. It's not 100% kosher, but it happens to work because this only actually happens with 'int' in this build.

We need a scheme to box raw pointers into atomics (like atomic_ref will need) and to unbox atomic pointers to their raw types, to call this API.

Maybe in a later patch?


================
Comment at: include/atomic:1585
+template <class _Tp>
+_LIBCPP_INLINE_VISIBILITY void __cxx_atomic_notify_one(__cxx_atomic_impl<_Tp> const volatile*) {
+}
----------------
zoecarver wrote:
> This will just silently fail if `_LIBCPP_HAS_NO_THREAD_CONTENTION_TABLE`. Is that OK?
It's not failing, it's that the implementation is to do nothing in that case.


================
Comment at: include/atomic:1597
+#ifndef _LIBCPP_NO_SPINNING
+    for(int __i = 0; __i < 16; ++__i) {
+        if(!__cxx_atomic_compare_equal(__cxx_atomic_load(__a, __order), __val))
----------------
zoecarver wrote:
> I'm curious, why `16` here? 
That could be a whole cppcon talk in its own right.

It's a magic number. Values in this neighborhood have the best performance on most systems, over a really wide range of systems, like from ARM SOCs to IBM supercomputer nodes.

I abstracted it into a macro for the next patch. Then polling could be even disabled by setting it to 0.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480





More information about the libcxx-commits mailing list