Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb abandoned this revision.


Comment at: lib/Frontend/InitPreprocessor.cpp:465
@@ +464,3 @@
+  if (LangOpts.CPlusPlus1z) {
+Builder.defineMacro("__cpp_lib_atomic_is_always_lock_free", "201603");
+  }

jfb wrote:
> rsmith wrote:
> > This should be defined by the relevant library header, not by the compiler, 
> > to indicate the library actually provides the new symbol.
> Ah yes, makes sense. Looks like libc++ doesn't do this for other library 
> features yet so I'll add it there.
> 
> This patch would then just fix test/Lexer/cxx-features.cpp for C++17 support 
> and have nothing to do with `is_always_lock_free`. It's probably better if I 
> close this patch and open a new one in that case.
On the mailing list @rsmith said:

> Feel free to commit that portion of the change.

Done in: http://reviews.llvm.org/rL264098

I'll abandon this change since it doesn't contain anything useful anymore (all 
in libc++ instead).

Thanks for the reviews!


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Richard Smith via cfe-commits]

On Mon, Mar 21, 2016 at 4:46 PM, JF Bastien via cfe-commits
 wrote:
> jfb added inline comments.
>
> 
> Comment at: lib/Frontend/InitPreprocessor.cpp:465
> @@ +464,3 @@
> +  if (LangOpts.CPlusPlus1z) {
> +Builder.defineMacro("__cpp_lib_atomic_is_always_lock_free", "201603");
> +  }
> 
> rsmith wrote:
>> This should be defined by the relevant library header, not by the compiler, 
>> to indicate the library actually provides the new symbol.
> Ah yes, makes sense. Looks like libc++ doesn't do this for other library 
> features yet so I'll add it there.
>
> This patch would then just fix test/Lexer/cxx-features.cpp for C++17 support 
> and have nothing to do with `is_always_lock_free`. It's probably better if I 
> close this patch and open a new one in that case.

Feel free to commit that portion of the change.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added inline comments.


Comment at: lib/Frontend/InitPreprocessor.cpp:465
@@ +464,3 @@
+  if (LangOpts.CPlusPlus1z) {
+Builder.defineMacro("__cpp_lib_atomic_is_always_lock_free", "201603");
+  }

rsmith wrote:
> This should be defined by the relevant library header, not by the compiler, 
> to indicate the library actually provides the new symbol.
Ah yes, makes sense. Looks like libc++ doesn't do this for other library 
features yet so I'll add it there.

This patch would then just fix test/Lexer/cxx-features.cpp for C++17 support 
and have nothing to do with `is_always_lock_free`. It's probably better if I 
close this patch and open a new one in that case.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/Frontend/InitPreprocessor.cpp:465
@@ +464,3 @@
+  if (LangOpts.CPlusPlus1z) {
+Builder.defineMacro("__cpp_lib_atomic_is_always_lock_free", "201603");
+  }

This should be defined by the relevant library header, not by the compiler, to 
indicate the library actually provides the new symbol.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[James Y Knight via cfe-commits]

jyknight added a comment.

> Changed to what you suggested. Much nicer. I don't remember why I thought it 
> was a bad idea.


Thanks, great! I don't have any opinion on what remains in this patch; someone 
else should review now.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Joerg Sonnenberger via cfe-commits]

On Fri, Mar 18, 2016 at 12:11:22PM -0500, Craig, Ben via cfe-commits wrote:
> A lot of it is a frontend decision.  What goes in the libcall feels an awful
> lot like the 386 vs 486 example that I hear a lot.  If I want one binary
> that can run on both a 386 (very limited atomic support) and a 486 (a bit
> more atomic support), then I generate the binary such that it has library
> calls to atomic routines.  The library on the 386 uses the global lock
> shard, and the library on the 486 uses native instructions.  The same kind
> of thing would apply for one byte atomics when only four byte atomics have
> hardware support.  The library call would use the lock shard as the default.
> If the platform doesn't have byte granularity memory protection a different,
> lockless implementation could be used that loads the surrounding four bytes
> instead and does all the masking.

The situation here is quite different. If you have a way to do lock-free
CAS for 32bit, you can implement it for 16bit and 8bit as well. On other
hand, if you do not have a lock-free CAS of a given size, it can be
implemented as lock-free libcall only if (a) no such MP configuration
exists and (b) (non-atomic) load/store of that size is supported. I'm
not even sure if such a thing as a SMP i386 ever existed, given that the
APIC specifications are all for i486 and later.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Craig, Ben via cfe-commits]

On 3/18/2016 1:50 AM, Joerg Sonnenberger via cfe-commits wrote:

On Thu, Mar 17, 2016 at 05:56:17PM +, JF Bastien via cfe-commits wrote:

C++ atomics are explicitly designed to avoid problems with touching
adjacent bytes: if `atomic` where `sizeof(T) == 1` requires a 4-byte
`cmpxchg` then it's up to the frontend to make sure `sizeof >= 4`
(or something equivalent such as making it non-lock-free).

That's not completely relevant for the code at hand. At least the
__sync_* builtins only ever required appropiate alignment of the base
type, not necessarily extra alignment to avoid padding for stupid
codegen. I also believe that access to extra data is completely harmless
for all but one use case. If you are directly accessing memory mapped
devices using compiler atomics, you should really know what you are
doing. That's about the only case where it matters.
Some architectures support byte granularity memory protection. Accessing 
unsolicited bytes can cause a trap on those architectures.


I think it makes sense for atomic and Atomic to get turned 
into the 4 byte __atomic intrinsics.  Those 4 byte __atomic intrinsics 
can then get lowered to efficient inlined code. If someone has a regular 
char, and they use the __atomic or __sync intrinsics directly, I'm fine 
with that going to a lib call.  The lib call can then either use the 
global lock shard, or access extra data, depending on what is reasonable 
for that platform.


-- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation 
Center, Inc. is a member of Code Aurora Forum, a Linux Foundation 
Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added a comment.

In http://reviews.llvm.org/D17950#376965, @jfb wrote:

> In http://reviews.llvm.org/D17950#376349, @jyknight wrote:
>
> > This conflicts with http://reviews.llvm.org/D17933. Most of this change 
> > also seems unnecessary.
> >
> > - I think the `is_always_lock_free` function should be defined based on the 
> > existing `__atomic_always_lock_free` builtin, not on defines (just like 
> > is_lock_free uses `__atomic_is_lock_free`, or `__c11_atomic_is_lock_free`, 
> > which is effectively an alias).
> > - Then, the new `__GCC_ATOMIC_DOUBLE_LOCK_FREE` macros are unnecessary, 
> > unless we need to actually define a `ATOMIC_DOUBLE_LOCK_FREE` macro.
> > - `__LLVM_ATOMIC_1_BYTES_LOCK_FREE` effectively duplicates 
> > `__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1`, so aren't needed.
>
>
> Hmm, when I originally wrote the paper I though I'd tried that. Can't 
> remember why I went the other way, let me try out 
> `__atomic_always_lock_free`. That would indeed be much simpler as it would be 
> a pure libc++ change., thanks for raising the issue.


Changed to what you suggested. Much nicer. I don't remember why I thought it 
was a bad idea.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Joerg Sonnenberger via cfe-commits]

On Thu, Mar 17, 2016 at 05:56:17PM +, JF Bastien via cfe-commits wrote:
> C++ atomics are explicitly designed to avoid problems with touching
> adjacent bytes: if `atomic` where `sizeof(T) == 1` requires a 4-byte
> `cmpxchg` then it's up to the frontend to make sure `sizeof >= 4`
> (or something equivalent such as making it non-lock-free).

That's not completely relevant for the code at hand. At least the
__sync_* builtins only ever required appropiate alignment of the base
type, not necessarily extra alignment to avoid padding for stupid
codegen. I also believe that access to extra data is completely harmless
for all but one use case. If you are directly accessing memory mapped
devices using compiler atomics, you should really know what you are
doing. That's about the only case where it matters.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Ben Craig via cfe-commits]

bcraig added a subscriber: bcraig.


Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+TypeWidth <= InlineWidth)
+  return Always;

On some targets (like Hexagon), 4-byte values are cheap to inline, but 1-byte 
values are not.  Clang is spotty about checking this, but 
TargetInfo::hasBuiltinAtomic seems like the right function to ask, if you have 
access to it.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Ben Craig via cfe-commits]

bcraig added inline comments.


Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+TypeWidth <= InlineWidth)
+  return Always;

jfb wrote:
> bcraig wrote:
> > jyknight wrote:
> > > jfb wrote:
> > > > bcraig wrote:
> > > > > On some targets (like Hexagon), 4-byte values are cheap to inline, 
> > > > > but 1-byte values are not.  Clang is spotty about checking this, but 
> > > > > TargetInfo::hasBuiltinAtomic seems like the right function to ask, if 
> > > > > you have access to it.
> > > > You're commenting on:
> > > > ```
> > > > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
> > > > TypeWidth <= InlineWidth)
> > > > ```
> > > > ?
> > > > 
> > > > Are you asking for a separate change, or for a change to my patch?
> > > That issue in clang's purview in any case; if hexagon wants to lower a 
> > > 1-byte cmpxchg to a compiler runtime function, it should do so in its 
> > > backend.
> > I am asking for a change to this patch.  Don't assume that all widths 
> > smaller than some value are inline-able and lock free, because that may not 
> > be the case.  A 4 byte cmpxchg could be lock free, while a 1 byte cmpxchg 
> > may not be lock free.
> Are you asking me to change this line:
> ```
> if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
> TypeWidth <= InlineWidth)
> ```
> ?
> 
> In what way?
> 
> 
> Please be more specific.
Yes, please change this line...

 if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
 TypeWidth <= InlineWidth)

... to something more like ...
 if(TI.hasBuiltinAtomic(TypeWidth, TypeAlign))

You will need to get the TargetInfo class into the method, and you should 
ensure that TypeWidth and TypeAlign are measured in terms of bits, because that 
is what TypeInfo::hasBuiltinAtomic is expecting.

Using TargetInfo::hasBuiltinAtomic will let Targets have explicit control over 
what is and is not always lock free.  The default implementation also happens 
to be pretty reasonable.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added a comment.

In http://reviews.llvm.org/D17950#377386, @cfe-commits wrote:

> I know that MIPS does that, and an out-of-tree implementation of hexagon 
>  implements 1-byte cmpxchg in terms of the 4-byte version. The emulation 
>  code isn't particularly small, and it seems reasonable to make it a 
>  libcall.  The emulation code seems sketchy from a correctness 
>  perspective, as you end up generating unsolicited loads and stores on 
>  adjacent bytes.  Take a look at the thread on cfe-dev and llvm-dev named 
>  "the as-if rule / perf vs. security" for some of the ramifications and 
>  concerns surrounding unsolicited loads and stores.


C++ atomics are explicitly designed to avoid problems with touching adjacent 
bytes: if `atomic` where `sizeof(T) == 1` requires a 4-byte `cmpxchg` then 
it's up to the frontend to make sure `sizeof >= 4` (or something 
equivalent such as making it non-lock-free).

This doesn't fall under "as-if".

Whether clang does this correctly for the targets you have in mind is another 
issue.

Whether LLVM's atomic instructions should assume C++ semantics in one way or 
another, i.e. whether they should assume the frontend generated code where 
padding can be overriden, is also a separate issue.

In all of these cases I suggest filing a separate issue. This patch is the 
wrong place to address the issues you raise. They're not invalid issues, 
they're simply not related to http://reviews.llvm.org/D17950.



Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+TypeWidth <= InlineWidth)
+  return Always;

bcraig wrote:
> jfb wrote:
> > bcraig wrote:
> > > jyknight wrote:
> > > > jfb wrote:
> > > > > bcraig wrote:
> > > > > > On some targets (like Hexagon), 4-byte values are cheap to inline, 
> > > > > > but 1-byte values are not.  Clang is spotty about checking this, 
> > > > > > but TargetInfo::hasBuiltinAtomic seems like the right function to 
> > > > > > ask, if you have access to it.
> > > > > You're commenting on:
> > > > > ```
> > > > > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 
> > > > > &&
> > > > > TypeWidth <= InlineWidth)
> > > > > ```
> > > > > ?
> > > > > 
> > > > > Are you asking for a separate change, or for a change to my patch?
> > > > That issue in clang's purview in any case; if hexagon wants to lower a 
> > > > 1-byte cmpxchg to a compiler runtime function, it should do so in its 
> > > > backend.
> > > I am asking for a change to this patch.  Don't assume that all widths 
> > > smaller than some value are inline-able and lock free, because that may 
> > > not be the case.  A 4 byte cmpxchg could be lock free, while a 1 byte 
> > > cmpxchg may not be lock free.
> > Are you asking me to change this line:
> > ```
> > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
> > TypeWidth <= InlineWidth)
> > ```
> > ?
> > 
> > In what way?
> > 
> > 
> > Please be more specific.
> Yes, please change this line...
> 
>  if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
>  TypeWidth <= InlineWidth)
> 
> ... to something more like ...
>  if(TI.hasBuiltinAtomic(TypeWidth, TypeAlign))
> 
> You will need to get the TargetInfo class into the method, and you should 
> ensure that TypeWidth and TypeAlign are measured in terms of bits, because 
> that is what TypeInfo::hasBuiltinAtomic is expecting.
> 
> Using TargetInfo::hasBuiltinAtomic will let Targets have explicit control 
> over what is and is not always lock free.  The default implementation also 
> happens to be pretty reasonable.
That's entirely unrelated to my change. My change is designed to *support* the 
use case you allude to even though right now LLVM doesn't have targets which 
exercise this usecase.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Craig, Ben via cfe-commits]

I know that MIPS does that, and an out-of-tree implementation of hexagon 
implements 1-byte cmpxchg in terms of the 4-byte version. The emulation 
code isn't particularly small, and it seems reasonable to make it a 
libcall.  The emulation code seems sketchy from a correctness 
perspective, as you end up generating unsolicited loads and stores on 
adjacent bytes.  Take a look at the thread on cfe-dev and llvm-dev named 
"the as-if rule / perf vs. security" for some of the ramifications and 
concerns surrounding unsolicited loads and stores.


On 3/17/2016 10:05 AM, James Y Knight wrote:


> A 4 byte cmpxchg could be lock free, while a 1 byte cmpxchg may not 
be lock free.


That's not possible: if you have a 4-byte cmpxchg instruction, you can 
use it to implement a 1-byte cmpxchg, too. Many targets do this 
already. (It would be better if that was available generically so that 
code didn't need to be written separately fit each target, but that's 
not the case at the moment.)




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[James Y Knight via cfe-commits]

jyknight added a subscriber: jyknight.
jyknight added a comment.

This conflicts with http://reviews.llvm.org/D17933. Most of this change also 
seems unnecessary.

- I think the `is_always_lock_free` function should be defined based on the 
existing `__atomic_always_lock_free` builtin, not on defines (just like 
is_lock_free uses `__atomic_is_lock_free`, or `__c11_atomic_is_lock_free`, 
which is effectively an alias).
- Then, the new `__GCC_ATOMIC_DOUBLE_LOCK_FREE` macros are unnecessary, unless 
we need to actually define a `ATOMIC_DOUBLE_LOCK_FREE` macro.
- `__LLVM_ATOMIC_1_BYTES_LOCK_FREE` effectively duplicates 
`__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1`, so aren't needed.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb updated this revision to Diff 51048.
jfb added a comment.

- Use __atomic_always_lock_free instead


http://reviews.llvm.org/D17950

Files:
  lib/Frontend/InitPreprocessor.cpp
  test/Lexer/cxx-features.cpp

Index: test/Lexer/cxx-features.cpp
===
--- test/Lexer/cxx-features.cpp
+++ test/Lexer/cxx-features.cpp
@@ -1,133 +1,141 @@
 // RUN: %clang_cc1 -std=c++98 -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 // RUN: %clang_cc1 -std=c++1y -fsized-deallocation -verify %s
-// RUN: %clang_cc1 -std=c++1y -fsized-deallocation -fconcepts-ts -DCONCEPTS_TS=1 -verify %s
+// RUN: %clang_cc1 -std=c++14 -fsized-deallocation -verify %s
+// RUN: %clang_cc1 -std=c++1z -fsized-deallocation -verify %s
+// RUN: %clang_cc1 -std=c++1z -fsized-deallocation -fconcepts-ts -DCONCEPTS_TS=1 -verify %s
 // RUN: %clang_cc1 -fcoroutines -DCOROUTINES -verify %s
 
 // expected-no-diagnostics
 
 // FIXME using `defined` in a macro has undefined behavior.
 #if __cplusplus < 201103L
-#define check(macro, cxx98, cxx11, cxx1y) cxx98 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx98
-#elif __cplusplus < 201304L
-#define check(macro, cxx98, cxx11, cxx1y) cxx11 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx11
+#define check(macro, cxx98, cxx11, cxx14, cxx1z) cxx98 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx98
+#elif __cplusplus < 201402L
+#define check(macro, cxx98, cxx11, cxx14, cxx1z) cxx11 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx11
+#elif __cplusplus < 201406L
+#define check(macro, cxx98, cxx11, cxx14, cxx1z) cxx14 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx14
 #else
-#define check(macro, cxx98, cxx11, cxx1y) cxx1y == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx1y
+#define check(macro, cxx98, cxx11, cxx14, cxx1z) cxx1z == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx1z
 #endif
 
-#if check(binary_literals, 0, 0, 201304)
+#if check(binary_literals, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_binary_literals"
 #endif
 
-#if check(digit_separators, 0, 0, 201309)
+#if check(digit_separators, 0, 0, 201309, 201309)
 #error "wrong value for __cpp_digit_separators"
 #endif
 
-#if check(init_captures, 0, 0, 201304)
+#if check(init_captures, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_init_captures"
 #endif
 
-#if check(generic_lambdas, 0, 0, 201304)
+#if check(generic_lambdas, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_generic_lambdas"
 #endif
 
-#if check(sized_deallocation, 0, 0, 201309)
+#if check(sized_deallocation, 0, 0, 201309, 201309)
 #error "wrong value for __cpp_sized_deallocation"
 #endif
 
-#if check(constexpr, 0, 200704, 201304)
+#if check(constexpr, 0, 200704, 201304, 201304)
 #error "wrong value for __cpp_constexpr"
 #endif
 
-#if check(decltype_auto, 0, 0, 201304)
+#if check(decltype_auto, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_decltype_auto"
 #endif
 
-#if check(return_type_deduction, 0, 0, 201304)
+#if check(return_type_deduction, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_return_type_deduction"
 #endif
 
-#if check(runtime_arrays, 0, 0, 0)
+#if check(runtime_arrays, 0, 0, 0, 0)
 #error "wrong value for __cpp_runtime_arrays"
 #endif
 
-#if check(aggregate_nsdmi, 0, 0, 201304)
+#if check(aggregate_nsdmi, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_aggregate_nsdmi"
 #endif
 
-#if check(variable_templates, 0, 0, 201304)
+#if check(variable_templates, 0, 0, 201304, 201304)
 #error "wrong value for __cpp_variable_templates"
 #endif
 
-#if check(unicode_characters, 0, 200704, 200704)
+#if check(unicode_characters, 0, 200704, 200704, 200704)
 #error "wrong value for __cpp_unicode_characters"
 #endif
 
-#if check(raw_strings, 0, 200710, 200710)
+#if check(raw_strings, 0, 200710, 200710, 200710)
 #error "wrong value for __cpp_raw_strings"
 #endif
 
-#if check(unicode_literals, 0, 200710, 200710)
+#if check(unicode_literals, 0, 200710, 200710, 200710)
 #error "wrong value for __cpp_unicode_literals"
 #endif
 
-#if check(user_defined_literals, 0, 200809, 200809)
+#if check(user_defined_literals, 0, 200809, 200809, 200809)
 #error "wrong value for __cpp_user_defined_literals"
 #endif
 
-#if check(lambdas, 0, 200907, 200907)
+#if check(lambdas, 0, 200907, 200907, 200907)
 #error "wrong value for __cpp_lambdas"
 #endif
 
-#if check(range_based_for, 0, 200907, 200907)
+#if check(range_based_for, 0, 200907, 200907, 200907)
 #error "wrong value for __cpp_range_based_for"
 #endif
 
-#if check(static_assert, 0, 200410, 200410)
+#if check(static_assert, 0, 200410, 200410, 200410)
 #error "wrong value for __cpp_static_assert"
 #endif
 
-#if check(decltype, 0, 200707, 200707)
+#if check(decltype, 0, 200707, 200707, 200707)
 #error "wrong value for __cpp_decltype"
 #endif
 
-#if check(attributes, 0, 200809, 200809)
+#if check(attributes, 0, 200809, 200809, 200809)
 #error "wrong value for __cpp_attributes"
 #endif
 
-#if check(rvalue_references, 0, 200610, 200610)
+#if 

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added inline comments.


Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+TypeWidth <= InlineWidth)
+  return Always;

bcraig wrote:
> jyknight wrote:
> > jfb wrote:
> > > bcraig wrote:
> > > > On some targets (like Hexagon), 4-byte values are cheap to inline, but 
> > > > 1-byte values are not.  Clang is spotty about checking this, but 
> > > > TargetInfo::hasBuiltinAtomic seems like the right function to ask, if 
> > > > you have access to it.
> > > You're commenting on:
> > > ```
> > > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
> > > TypeWidth <= InlineWidth)
> > > ```
> > > ?
> > > 
> > > Are you asking for a separate change, or for a change to my patch?
> > That issue in clang's purview in any case; if hexagon wants to lower a 
> > 1-byte cmpxchg to a compiler runtime function, it should do so in its 
> > backend.
> I am asking for a change to this patch.  Don't assume that all widths smaller 
> than some value are inline-able and lock free, because that may not be the 
> case.  A 4 byte cmpxchg could be lock free, while a 1 byte cmpxchg may not be 
> lock free.
Are you asking me to change this line:
```
if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
TypeWidth <= InlineWidth)
```
?

In what way?


Please be more specific.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[Ben Craig via cfe-commits]

bcraig added inline comments.


Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+TypeWidth <= InlineWidth)
+  return Always;

jyknight wrote:
> jfb wrote:
> > bcraig wrote:
> > > On some targets (like Hexagon), 4-byte values are cheap to inline, but 
> > > 1-byte values are not.  Clang is spotty about checking this, but 
> > > TargetInfo::hasBuiltinAtomic seems like the right function to ask, if you 
> > > have access to it.
> > You're commenting on:
> > ```
> > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
> > TypeWidth <= InlineWidth)
> > ```
> > ?
> > 
> > Are you asking for a separate change, or for a change to my patch?
> That issue in clang's purview in any case; if hexagon wants to lower a 1-byte 
> cmpxchg to a compiler runtime function, it should do so in its backend.
I am asking for a change to this patch.  Don't assume that all widths smaller 
than some value are inline-able and lock free, because that may not be the 
case.  A 4 byte cmpxchg could be lock free, while a 1 byte cmpxchg may not be 
lock free.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[James Y Knight via cfe-commits]

On Thu, Mar 17, 2016 at 12:55 PM, Craig, Ben 
wrote:

> I know that MIPS does that, and an out-of-tree implementation of hexagon
> implements 1-byte cmpxchg in terms of the 4-byte version. The emulation
> code isn't particularly small, and it seems reasonable to make it a libcall.


That's fine. Either do it inline or via an out-of-line version of the same
code, in a libcall. Either way acts the same, that is something that the
target backend can decide to do whenever it wants.

The important property for Clang is that it know whether it's lock-free or
not. And I don't believe there's any reason to use a mutex to implement a
1-byte ,cmpxchg on a platform with a 4-byte cmpxchg instruction available.

  The emulation code seems sketchy from a correctness perspective, as you
> end up generating unsolicited loads and stores on adjacent bytes.


I disagree. I do not see how expanding a subword cmpxchg into a
word-size-and-aligned cmpxchg could possibly cause a problem.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added a comment.

In http://reviews.llvm.org/D17950#376349, @jyknight wrote:

> This conflicts with http://reviews.llvm.org/D17933. Most of this change also 
> seems unnecessary.
>
> - I think the `is_always_lock_free` function should be defined based on the 
> existing `__atomic_always_lock_free` builtin, not on defines (just like 
> is_lock_free uses `__atomic_is_lock_free`, or `__c11_atomic_is_lock_free`, 
> which is effectively an alias).
> - Then, the new `__GCC_ATOMIC_DOUBLE_LOCK_FREE` macros are unnecessary, 
> unless we need to actually define a `ATOMIC_DOUBLE_LOCK_FREE` macro.
> - `__LLVM_ATOMIC_1_BYTES_LOCK_FREE` effectively duplicates 
> `__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1`, so aren't needed.


Hmm, when I originally wrote the paper I though I'd tried that. Can't remember 
why I went the other way, let me try out `__atomic_always_lock_free`. That 
would indeed be much simpler as it would be a pure libc++ change., thanks for 
raising the issue.



Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+TypeWidth <= InlineWidth)
+  return Always;

bcraig wrote:
> On some targets (like Hexagon), 4-byte values are cheap to inline, but 1-byte 
> values are not.  Clang is spotty about checking this, but 
> TargetInfo::hasBuiltinAtomic seems like the right function to ask, if you 
> have access to it.
You're commenting on:
```
if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
TypeWidth <= InlineWidth)
```
?

Are you asking for a separate change, or for a change to my patch?


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[James Y Knight via cfe-commits]

> A 4 byte cmpxchg could be lock free, while a 1 byte cmpxchg may not be
lock free.

That's not possible: if you have a 4-byte cmpxchg instruction, you can use
it to implement a 1-byte cmpxchg, too. Many targets do this already. (It
would be better if that was available generically so that code didn't need
to be written separately fit each target, but that's not the case at the
moment.)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added inline comments.


Comment at: lib/Frontend/InitPreprocessor.cpp:480
@@ +479,3 @@
+  if (LangOpts.CPlusPlus1z) {
+Builder.defineMacro("__cpp_lib_atomic_is_always_lock_free", "201603");
+  }

How do you pick these numbers before the standard is actually out? 201603 is 
the Jacksonville meeting, seems fine to me :)


Comment at: lib/Frontend/InitPreprocessor.cpp:853
@@ -830,1 +852,3 @@
+DEFINE_LOCK_FREE_MACRO(DOUBLE, Double);
+DEFINE_LOCK_FREE_MACRO(LDOUBLE, LongDouble);
 Builder.defineMacro("__GCC_ATOMIC_POINTER_LOCK_FREE",

Adding these which aren't in the C standard, but which `is_always_lock_free` 
would use. We're proposing that floating-point atomics be added so it's not 
crazy:
  http://open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0020r1.html


Comment at: test/Lexer/cxx-features.cpp:6
@@ -5,1 +5,3 @@
+// RUN: %clang_cc1 -std=c++1z -fsized-deallocation -verify %s
+// RUN: %clang_cc1 -std=c++1z -fsized-deallocation -fconcepts-ts 
-DCONCEPTS_TS=1 -verify %s
 // RUN: %clang_cc1 -fcoroutines -DCOROUTINES -verify %s

This was out of date, I'm updating the test to C++1z, testing C++14, and 
keeping c++1y for compatibility.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb added a comment.

I'll check with GCC folks whether they want to share macros the same way we're 
already sharing `__GCC_ATOMIC_*_LOCK_FREE`. The new macros I propose are 
`__LLVM__ATOMIC_*_BYTES_LOCK_FREE` and `__LLVM_LOCK_FREE_IS_SIZE_BASED`.

Let's do a first sanity-check round of code reviews to make sure I have the 
LLVM-isms down right, then I'll add GCC folks.


http://reviews.llvm.org/D17950



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

jfb created this revision.
jfb added reviewers: mclow.lists, rsmith.
jfb added a subscriber: cfe-commits.
Herald added subscribers: dschuff, jfb.

This was voted into C++17 at last week's Jacksonville meeting. The final 
P0152R1 paper will be in the upcoming post-Jacksonville mailing, and is also 
available here:
  http://jfbastien.github.io/papers/P0152R1.html

The libc++ part of this implementation is in the following code review:
  

http://reviews.llvm.org/D17950

Files:
  lib/Frontend/InitPreprocessor.cpp
  test/Lexer/cxx-features.cpp
  test/Preprocessor/init.c

Index: test/Preprocessor/init.c
===
--- test/Preprocessor/init.c
+++ test/Preprocessor/init.c
@@ -3282,7 +3282,10 @@
 // MIPSN32BE: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
 // MIPSN32BE: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
 // MIPSN32BE: #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
+// MIPSN32BE: #define __GCC_ATOMIC_DOUBLE_LOCK_FREE 2
+// MIPSN32BE: #define __GCC_ATOMIC_FLOAT_LOCK_FREE 2
 // MIPSN32BE: #define __GCC_ATOMIC_INT_LOCK_FREE 2
+// MIPSN32BE: #define __GCC_ATOMIC_LDOUBLE_LOCK_FREE 1
 // MIPSN32BE: #define __GCC_ATOMIC_LLONG_LOCK_FREE 2
 // MIPSN32BE: #define __GCC_ATOMIC_LONG_LOCK_FREE 2
 // MIPSN32BE: #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
@@ -3372,6 +3375,12 @@
 // MIPSN32BE: #define __LDBL_MIN_10_EXP__ (-4931)
 // MIPSN32BE: #define __LDBL_MIN_EXP__ (-16381)
 // MIPSN32BE: #define __LDBL_MIN__ 3.36210314311209350626267781732175260e-4932L
+// MIPSN32BE: #define __LLVM_ATOMIC_16_BYTES_LOCK_FREE 1
+// MIPSN32BE: #define __LLVM_ATOMIC_1_BYTES_LOCK_FREE 2
+// MIPSN32BE: #define __LLVM_ATOMIC_2_BYTES_LOCK_FREE 2
+// MIPSN32BE: #define __LLVM_ATOMIC_4_BYTES_LOCK_FREE 2
+// MIPSN32BE: #define __LLVM_ATOMIC_8_BYTES_LOCK_FREE 2
+// MIPSN32BE: #define __LLVM_LOCK_FREE_IS_SIZE_BASED 1
 // MIPSN32BE: #define __LONG_LONG_MAX__ 9223372036854775807LL
 // MIPSN32BE: #define __LONG_MAX__ 2147483647L
 // MIPSN32BE: #define __MIPSEB 1
@@ -3587,7 +3596,10 @@
 // MIPSN32EL: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
 // MIPSN32EL: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
 // MIPSN32EL: #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
+// MIPSN32EL: #define __GCC_ATOMIC_DOUBLE_LOCK_FREE 2
+// MIPSN32EL: #define __GCC_ATOMIC_FLOAT_LOCK_FREE 2
 // MIPSN32EL: #define __GCC_ATOMIC_INT_LOCK_FREE 2
+// MIPSN32EL: #define __GCC_ATOMIC_LDOUBLE_LOCK_FREE 1
 // MIPSN32EL: #define __GCC_ATOMIC_LLONG_LOCK_FREE 2
 // MIPSN32EL: #define __GCC_ATOMIC_LONG_LOCK_FREE 2
 // MIPSN32EL: #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
@@ -3678,6 +3690,12 @@
 // MIPSN32EL: #define __LDBL_MIN_EXP__ (-16381)
 // MIPSN32EL: #define __LDBL_MIN__ 3.36210314311209350626267781732175260e-4932L
 // MIPSN32EL: #define __LITTLE_ENDIAN__ 1
+// MIPSN32EL: #define __LLVM_ATOMIC_16_BYTES_LOCK_FREE 1
+// MIPSN32EL: #define __LLVM_ATOMIC_1_BYTES_LOCK_FREE 2
+// MIPSN32EL: #define __LLVM_ATOMIC_2_BYTES_LOCK_FREE 2
+// MIPSN32EL: #define __LLVM_ATOMIC_4_BYTES_LOCK_FREE 2
+// MIPSN32EL: #define __LLVM_ATOMIC_8_BYTES_LOCK_FREE 2
+// MIPSN32EL: #define __LLVM_LOCK_FREE_IS_SIZE_BASED 1
 // MIPSN32EL: #define __LONG_LONG_MAX__ 9223372036854775807LL
 // MIPSN32EL: #define __LONG_MAX__ 2147483647L
 // MIPSN32EL: #define __MIPSEL 1
@@ -7614,7 +7632,10 @@
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_CHAR_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __GCC_ATOMIC_DOUBLE_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __GCC_ATOMIC_FLOAT_LOCK_FREE 2
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_INT_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __GCC_ATOMIC_LDOUBLE_LOCK_FREE 2
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_LLONG_LOCK_FREE 2
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_LONG_LOCK_FREE 2
 // X86_64-CLOUDABI:#define __GCC_ATOMIC_POINTER_LOCK_FREE 2
@@ -7705,6 +7726,12 @@
 // X86_64-CLOUDABI:#define __LDBL_MIN_EXP__ (-16381)
 // X86_64-CLOUDABI:#define __LDBL_MIN__ 3.36210314311209350626e-4932L
 // X86_64-CLOUDABI:#define __LITTLE_ENDIAN__ 1
+// X86_64-CLOUDABI:#define __LLVM_ATOMIC_16_BYTES_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __LLVM_ATOMIC_1_BYTES_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __LLVM_ATOMIC_2_BYTES_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __LLVM_ATOMIC_4_BYTES_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __LLVM_ATOMIC_8_BYTES_LOCK_FREE 2
+// X86_64-CLOUDABI:#define __LLVM_LOCK_FREE_IS_SIZE_BASED 1
 // X86_64-CLOUDABI:#define __LONG_LONG_MAX__ 9223372036854775807LL
 // X86_64-CLOUDABI:#define __LONG_MAX__ 9223372036854775807L
 // X86_64-CLOUDABI:#define __LP64__ 1
@@ -8478,7 +8505,10 @@
 // WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
 // WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
 // WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_CHAR_LOCK_FREE 2
+// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_DOUBLE_LOCK_FREE 1
+// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_FLOAT_LOCK_FREE 2
 // WEBASSEMBLY32-NEXT:#define