I've mentioned this before, but NVIDIA defines _VSTD to something else in our product branch. This diff, here, implies that we will need a matching & opposite diff to further separate us from upstream.

I've just read up the backstory of this.

Hmm. I lost track of how/when this constructor got defined as constexpr. It doesn't mean that it's unimplementable with platform semaphores, but to do so would make the implementation a lot more complicated, slower, and the object even larger than it already is.

This change looks good to me, thanks!

I'm drawing a blank. Proceed.

I think Jonathan is asking whether there is a match in the gray areas.

I'm not sold on this but I'm not deeply opposed either.

I've resumed looking at the library code.

We're looking at this patch in the context of the libcudacxx fork. I don't have more feedback right now but we probably will get there soon.

Also adding a test case.

I will revert this part immediately, thanks for the heads up!

This version is now clean on Mac and Linux

Removed spurious whitespace changes

I figured out I could abandon most of the noisy changes. Dropping them out.

I'm not an expert in the details but this looks right to me.

Yes! I can attest that the old text wasted a bunch of my time and the new text is what works.

What does the Linux syscall have to do with MUSL?

In D74918#1897636, @kristof.beyls wrote:

If these values are part of the C++ target platform ABI, it seems to me the values for std::hardware_{constructive,destructive}_interference_size should be set by whoever has the authority to decide C++ platform ABI for specific platforms.
Assuming my thought in the previous sentence is correct; discussions on which values to chose for std::hardware_{constructive,destructive}_interference_size should happen in whichever forums decide C++ platform ABI for the various platforms? (Maybe for some platforms that forum might be clang-related fora like reviews.llvm.org, but probably not for all platforms).
With my (probably limited) understanding of the requirements, it seems like deriving std::hardware_{constructive,destructive}_interference_size from actual cache line size on a specific micro-architecture doesn't seem to be the right approach?

(I assume I'm not seeing a code review being used to veto a C++ Standard feature, but actually the other points are the reason for the red flag.)

This revision addresses the outstanding comments, and incorporates Louis' recommended macros for ulock detection.

Replies. Also noting that I will include an update to the implementation status HTML page.

Thanks Zoe!

A lot of changes in this patch, mostly simplifications based on the areas that got feedback about complexity.

No problem. I just want to know what I need to do in the next patch in order to move forward.

That's a creative use of __cxx_atomic_impl that I hadn't foreseen. Seems to work. Ok.

In D71726#1792852, @yaxunl wrote:

In D71726#1791904, @jfb wrote:

This generally seems fine. Does it work on most backends? I want to make sure it doesn't fail in backends :)

For x86_64, amdgcn, aarch64, armv7, mips64, it is translated to cmpxchg by AtomicExpandPass and backends did codegen successfully.

For hexagon, riscv32, it is translated to call of __atomic_fetch_add_4 for fadd float. This is concerning. Probably we need to add __atomic_fetch_{add|sub}_{f16|f32|f64} ?

Hi guys. Could I get some action items or other movement on this review? Would you like me to make it possible to merge it as disabled or as experimental, and send me DRs to fix afterwards?

Closing out some obsolete comments after changes and/or testing.

This revision enables cross-ABI compatibility for building the dylib and the user application with different combinations of these options:

In this version I moved the tree barrier's core algorithm into the dylib so that any functional or performance issue found in it later could still be fixed without breaking ABI. By the same fact it narrows the visibility of the definition of the thread_local symbol it uses.

In this update I repaired the usefulness of <atomic> in C++03 mode. Other headers won't be supported there, but I'm avoiding regression to <atomic>.

Oof, didn't attach the patch.

Added some documentation for the semaphore and barrier algorithms as comments.

Fixed an incompatibility between the _Atomic(T) backend of <atomic> and the Futex function SFINAE in <__threading_support>.

Simplified how the contention state is conditionally-used.

Refactored the semaphores into layers that each do one clear thing, and dispensed with a lot of #ifdefs. Moved as much semaphore code as possible out of the header and into the dylib.

In D68480#1705203, @EricWF wrote:

Are the lock free algorithms used by this implementation published somewhere? That would give me a lot more confidence in their correctness.

Was missing an edit to the header CMakeLists to install the new headers.

Sorry for the delay posting the updated patch that addressed the recent comments, but I ran into some build issues.

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

My Mac tests didn't catch some Linux issues when I added __ almost everywhere.

Closed some of the comments that have been addressed.

This new patch has more context, more __, fewer escapes from the CUDA port.

Urg, sorry for struggling with UX here.

Cleaned up a dead macro

Removed some mentions of CUDA that slipped by

More cleaning

Cleaned up the patch a bit.

I think we want people to use -ffreestanding more for things like this, so the part where this is circumvented isn't clearly the right choice. The rest seems fine, and is exactly the reason why I created _LIBCPP_ATOMIC_ONLY_USE_BUILTINS.

In D62393#1542042, @tra wrote:

C++ volatile will give you that. You also rely on atomicity. C++ volatile does not guarantee that, even if NVPTX does happen to. It's a mere coincidence. What if next PTX revision provides a better way to implement C++ volatile without providing atomicity? Then your code will all of a sudden break in new and exciting ways.

In D62393#1542009, @ABataev wrote:

No, I'm not relying on the non-optimization of atomics. I need both, volatile semantics and atomic. So, the compiler could not optimize out memory accesses and the access would be atomic.

In D62393#1541976, @ABataev wrote:

In D62393#1541969, @tra wrote:

@reames , @tra , @Hahnfeld , @jlebar , @chandlerc, I see that this was discussed in D50391 (in terms of using PTX's volatile to provide atomic lowering), but it's not clear to me that using volatile with atomic semantics in LLVM is something we guarantee will work (even in CUDA mode). I imagine that we'd need to lower these in terms of relaxed atomics in Clang for this to really be guaranteed to work correctly.

Do I understand it correctly that the argument is about using C++ volatile with the assumption that those will map to ld.volatile/st.volatile, which happen to provide sufficiently strong guarantees to be equivalent of LLVM's atomic monotonic operations?

If that's the case, then I would agree that it's an implementation detail one should not be relying on. If atomicity is needed, we should figure out a way to express that in C++.

In practice, the standard C++ library support in cuda is somewhat limited. I don't think atomic<> works, so volatile might be a 'happens to work' short-term workaround. If that's the case, then there should a comment describing what's going on here and TODO to fix it when better support for atomics on GPU is available.

Another option would be to use atomic*() functions provided by CUDA, but those do have limited functionality on older GPUs.

Yet another alternative is to explicitly use ld.volatile/st.volatile from inline assembly. I don't think we have it plumbed as clang builtin.

Artem, thanks for your comment. But we need not only atomicity, bht also we need to command the ptxas compiler to not optimize accesses to parallelLevel. According to several comments, (like this one https://stackoverflow.com/a/1533115) it is recommended to use volatile modifier in Cuda in such cases. If clang does not provide the required level of support for Cuda volatile, I think this is an incompatibility with nvcc.
Also, I already thought about using PTX instructions directly. Probably, you're right that it would be better to use them if you're sure that there is a difference between nvcc and clang.

In D62393#1538879, @ABataev wrote:

In D62393#1538842, @__simt__ wrote:

I know some things about CUDA, volatile and C++. Let's see if I can understand the part of the proposed change that involves these. I don't understand the part about the test but I don't need to, I'll ignore that.

The gist of the issue is that parallelLevel table should really be atomic<> because of data-races on it (that was a bug prior to this, maybe there are more such bugs lying around), except there's a problem with the obvious fix: atomic<> is not available in CUDA (yet) so it's not an option to fix this issue. The next best thing we have instead is volatile.

Now, volatile in PTX (e.g. asm("ld.volatile...[x];")) and volatile (e.g. "volatile ...x;") in C++ source like this, are not exactly the same thing. When CUDA says that volatile is equivalent memory_order_relaxed, it's saying something clear (I think) about the PTX level code but it's still being pretty vague about CUDA C++ level code. OTOH it's entirely possible for Clang to do something with either atomic<> or volatile that isn't valid for the other -- and that's a different can of worms than, say, NVCC which does {whatever NVCC does, it's not the compiler you're using}.

However, since people do use CUDA C++ volatile this way a lot already, you can't really have a good CUDA toolchain unless it is the case (including, by accident) that it works for this purpose. In other words, it's probably reasonable to assume this in your code because every other code would be on fire otherwise, and it's not on fire, so far as we can tell.

It might be worth it to prepare your code for the eventual arrival of atomic<> on CUDA. That is, maybe create a template alias on T with some helper functions, just enough for your use. It might make this code more self-documenting and make it easy to make it 100% legit later on.

Hi Olivier, thanks for your comments. You're absolutely right. Actually, we're using both compilers, nvcc and clang (under different conditions, though). Marking the variable volatile does not break it in the LLVM level. Maybe, it is by accident, but I rather doubt in this.
Do you suggest to create a template function that will provide the access to the parallelLevel variable? Amd when the atomic<> is supported by Cuda change the type of this variable to atomic<> so the compiler could automatically instantiate this template function with the proper type, right? Or you have something different in mind? If so, could provide a small example of your idea?

I know some things about CUDA, volatile and C++. Let's see if I can understand the part of the proposed change that involves these. I don't understand the part about the test but I don't need to, I'll ignore that.

In D56913#1417172, @davide wrote:

This commit broke the atomic lldb data formatter.

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/21037/

@shafik @jingham

Hey guys, can I get additional feedback or else proceed with this patch?

Dear other reviewers, what else needs to happen here?

This version addresses the preceding comments and passes libcxx tests across c++03, 11, 14 and 17 modes.

Would it make sense to decide whether we want to use GCC's non-lockfree atomics or not based on a configuration macro that's not _LIBCPP_FREESTANDING?

This version passes libcxx tests with each combination of path that can be used (force GCC, force C11, force freestanding+non-lock-free). There were quite a few problems around volatile that I had not addressed yet, apologies.

I need to test the GCC path better, it still has some bugs. Be right back.

In this version:

I will come back with another patch that addresses these comments.

In this version I've restored the __cxx_atomic_... layer to which both the GCC and C11 backends map. This addresses the comment about introducing more functions named __c11_atomic... which are not part of the C11 builtin set. I do not introduce any new versions of _Atomic anymore.

Thanks for the comments, Louis, responses below.

In D56913#1379046, @jfb wrote:

Should all these macros be in __config?

Removed an inadvertent #define left in there for testing.

Would be better if it passed the libcxx tests with the feature turned on. Like now.

Fixed some spurious whitespace changes I didn't intend.

Simplified the changes significantly. By switching my back-end to slide under the C11 side instead of the GCC/Clang side, I can live without the new interposer layer.

Quick update before a longer update: I have a simpler patch on the way.

In D56913#1363801, @jfb wrote:

One downside to forwarding layers is that if you don't have an always inline attribute, some build might not inline. This leads to code bloat and crappy codegen because the memory_order attribute is no longer a known constexpr value. Maybe we should have always inline?

You don't define LIBCXX_FREESTANDING yet, right?

I think at a high level this looks fine.

Updated the patch with a bit higher-quality and better-tested code than what I originally showed.

With apologies, I would just like to tack a note into this thread that the entire *field* of formal memory model proofs involving partially-overlapping atomics is a single paper (last I knew, https://www.cl.cam.ac.uk/~pes20/popl17/mixed-size.pdf). The paper says mixed-size, but the key point is not-perfectly-overlapping.

Just a clarification - please evaluate the design aspects first. There are nits that I know are wrong and am still working on.

Hello Eric, everyone else,