This is an archive of the discontinued LLVM Phabricator instance.

Add __CLANG_ATOMIC_<TYPE>_LOCK_FREE macros for use in MSVC compatibility mode.
ClosedPublic

Authored by EricWF on Apr 19 2017, 8:26 PM.

Details

Summary

Libc++ currently implements the ATOMIC_<TYPE>_LOCK_FREE macros using the __GCC_ATOMIC_<TYPE>_LOCK_FREE macros. However these are not available when MSVC compatibility is enabled even though C11 _Atomic is. This prevents libc++ from correctly implementing ATOMIC_<TYPE>_LOCK_FREE.

This patch adds an alternative spelling __CLANG_ATOMIC_<TYPE>_LOCK_FREE that is enabled with -fms-compatibility.

Diff Detail

Event Timeline

EricWF created this revision.Apr 19 2017, 8:26 PM
smeenai resigned from this revision.Apr 19 2017, 8:52 PM
smeenai edited reviewers, added: compnerd; removed: smeenai.

This looks good to me, but I'm not familiar enough with this part of clang to be comfortable accepting.

EricWF added a reviewer: jfb.Apr 20 2017, 1:19 AM
jfb edited edge metadata.Apr 20 2017, 1:26 AM

Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL implement is_always_lock_free at the moment? CL 19 2017 RTW doesn't seem to have anything? Presumably they'll have to do *something*.

In D32265#731709, @jfb wrote:

Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL implement is_always_lock_free at the moment? CL 19 2017 RTW doesn't seem to have anything? Presumably they'll have to do *something*.

The goal is to allow libc++ to implement`ATOMIC_<TYPE>_LOCK_FREE` on Windows using Clang. As you know libc++ currently uses the __GCC_ATOMIC_FOO macros, but those aren't available on Windows.

Also AFAIK MSVC leaves the implementation of atomics up to the library, not the frontend. So W/E MSVC's STL does it's likely not sane or desirable.

EricWF added a reviewer: rnk.Apr 20 2017, 2:07 PM
jfb added a comment.Apr 20 2017, 2:15 PM
In D32265#731709, @jfb wrote:

Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL implement is_always_lock_free at the moment? CL 19 2017 RTW doesn't seem to have anything? Presumably they'll have to do *something*.

The goal is to allow libc++ to implement`ATOMIC_<TYPE>_LOCK_FREE` on Windows using Clang. As you know libc++ currently uses the __GCC_ATOMIC_FOO macros, but those aren't available on Windows.

OK so it's just a hygiene question of blanket-avoiding __GCC_* macros in clang-ms mode, but using __CLANG_* macros are OK? That sounds fine then. Would you all-out switch libc++ to use __CLANG_* then?

Also AFAIK MSVC leaves the implementation of atomics up to the library, not the frontend. So W/E MSVC's STL does it's likely not sane or desirable.

Wait what? How does that even work? This calls for a Twitter ping of Billy!

jfb added a comment.Apr 20 2017, 2:23 PM

It looks like Billy is going to do something somewhat similar when he rewrites it: https://twitter.com/jfbastien/status/855168230918307840
For now it's kinda #define IS_LOCK_FREE ¯\_(ツ)_/¯

In D32265#732649, @jfb wrote:
In D32265#731709, @jfb wrote:

Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL implement is_always_lock_free at the moment? CL 19 2017 RTW doesn't seem to have anything? Presumably they'll have to do *something*.

The goal is to allow libc++ to implement`ATOMIC_<TYPE>_LOCK_FREE` on Windows using Clang. As you know libc++ currently uses the __GCC_ATOMIC_FOO macros, but those aren't available on Windows.

OK so it's just a hygiene question of blanket-avoiding __GCC_* macros in clang-ms mode, but using __CLANG_* macros are OK? That sounds fine then. Would you all-out switch libc++ to use __CLANG_* then?

__CLANG macros are fine on libc++'s end; So as long as they're not uncouth for Clang it should be OK.

I would switch libc++ to do

#ifdef __CLANG_ATOMIC_FOO
# define ATOMIC_FOO __CLANG_ATOMIC_FOO
# define ATOMIC_BAR __CLANG_ATOMIC_BAR
#else
# define ATOMIC_FOO __GCC_ATOMIC_FOO
...
#endif

Also AFAIK MSVC leaves the implementation of atomics up to the library, not the frontend. So W/E MSVC's STL does it's likely not sane or desirable.

Wait what? How does that even work? This calls for a Twitter ping of Billy!

In D32265#732657, @jfb wrote:

It looks like Billy is going to do something somewhat similar when he rewrites it: https://twitter.com/jfbastien/status/855168230918307840
For now it's kinda #define IS_LOCK_FREE ¯\_(ツ)_/¯

Note that my rewrite of our <atomic> didn't touch the XXX_IS_LOCK_FREE macros. All of those for both implementations are hardcoded to 2 since any piece of hardware we ever plan to support has lock free ops for all those. I did add conditional DCAS support for amd64 though in v.Next.

Billy3

(sorry for double post)

And of course since you folks don't care about supporting pre-Vista you probably also don't care about supporting Opterons from 2005, in which case you'd want unconditional cmpxchg16b on amd64 :). In our next major version I added a configuration macro _STD_ATOMIC_ALWAYS_USE_CMPXCHG16B users can set to ask for that.

In D32265#731709, @jfb wrote:

Is it a goal to support Microsoft's STL with this? If so, how does MSVC's STL implement is_always_lock_free at the moment? CL 19 2017 RTW doesn't seem to have anything? Presumably they'll have to do *something*.

Our STL implements <atomic> entirely as a library feature (on top of the _InterlockedXxx intrinsics and alignas).

rsmith accepted this revision.Apr 20 2017, 3:35 PM

I'm not thrilled about adding yet more predefined macros, but it really doesn't make sense for libc++ to depend on __GCC_* macros when targeting Windows, nor for these to be Windows-only, so let's do it.

lib/Frontend/InitPreprocessor.cpp
910–912

I'd sink the _ into the prefix too, but this looks fine either way.

This revision is now accepted and ready to land.Apr 20 2017, 3:35 PM
EricWF updated this revision to Diff 96043.Apr 20 2017, 4:06 PM
  • Address inline comments.
EricWF closed this revision.Apr 20 2017, 4:06 PM