[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
algrant added a comment. There is a (stalled) proposal to add atomic integer max/min to C++: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0493r0.pdf . The proposal has memory semantics similar to these builtins, i.e. unconditional RMW. There is no limitation to 32-bit types though, what's the motivation for that? Is it a limitation of a particular target? Repository: rL LLVM https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332193: Added atomic_fetch_min, max, umin, umax intrinsics
to clang. (authored by delena, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D46386?vs=146462=146502#toc
Repository:
rL LLVM
https://reviews.llvm.org/D46386
Files:
cfe/trunk/docs/LanguageExtensions.rst
cfe/trunk/include/clang/Basic/Builtins.def
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/CodeGen/CGAtomic.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/CodeGen/Atomics.c
cfe/trunk/test/Sema/atomic-ops.c
Index: cfe/trunk/docs/LanguageExtensions.rst
===
--- cfe/trunk/docs/LanguageExtensions.rst
+++ cfe/trunk/docs/LanguageExtensions.rst
@@ -1975,6 +1975,32 @@
Support for constant expression evaluation for the above builtins be detected
with ``__has_feature(cxx_constexpr_string_builtins)``.
+Atomic Min/Max builtins with memory ordering
+
+
+There are two atomic builtins with min/max in-memory comparison and swap.
+The syntax and semantics are similar to GCC-compatible __atomic_* builtins.
+
+* ``__atomic_fetch_min``
+* ``__atomic_fetch_max``
+
+The builtins work with signed and unsigned integers and require to specify memory ordering.
+The return value is the original value that was stored in memory before comparison.
+
+Example:
+
+.. code-block:: c
+
+ unsigned int val = __atomic_fetch_min(unsigned int *pi, unsigned int ui, __ATOMIC_RELAXED);
+
+The third argument is one of the memory ordering specifiers ``__ATOMIC_RELAXED``,
+``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``,
+``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics.
+
+In terms or aquire-release ordering barriers these two operations are always
+considered as operations with *load-store* semantics, even when the original value
+is not actually modified after comparison.
+
.. _langext-__c11_atomic:
__c11_atomic builtins
@@ -2734,4 +2760,3 @@
The ``#pragma comment(lib, ...)`` directive is supported on all ELF targets.
The second parameter is the library name (without the traditional Unix prefix of
``lib``). This allows you to provide an implicit link of dependent libraries.
-
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7127,6 +7127,8 @@
def err_atomic_op_needs_atomic_int_or_ptr : Error<
"address argument to atomic operation must be a pointer to %select{|atomic }0"
"integer or pointer (%1 invalid)">;
+def err_atomic_op_needs_int32_or_ptr : Error<
+ "address argument to atomic operation must be a pointer to signed or unsigned 32-bit integer">;
def err_atomic_op_bitwise_needs_atomic_int : Error<
"address argument to bitwise atomic operation must be a pointer to "
"%select{|atomic }0integer (%1 invalid)">;
Index: cfe/trunk/include/clang/Basic/Builtins.def
===
--- cfe/trunk/include/clang/Basic/Builtins.def
+++ cfe/trunk/include/clang/Basic/Builtins.def
@@ -721,6 +721,10 @@
ATOMIC_BUILTIN(__opencl_atomic_fetch_min, "v.", "t")
ATOMIC_BUILTIN(__opencl_atomic_fetch_max, "v.", "t")
+// GCC does not support these, they are a Clang extension.
+ATOMIC_BUILTIN(__atomic_fetch_min, "iiD*i.", "t")
+ATOMIC_BUILTIN(__atomic_fetch_max, "v.", "t")
+
#undef ATOMIC_BUILTIN
// Non-overloaded atomic builtins.
Index: cfe/trunk/test/CodeGen/Atomics.c
===
--- cfe/trunk/test/CodeGen/Atomics.c
+++ cfe/trunk/test/CodeGen/Atomics.c
@@ -291,3 +291,10 @@
__sync_lock_release (); // CHECK: store atomic {{.*}} release, align 8
__sync_lock_release (); // CHECK: store atomic {{.*}} release, align 8
}
+
+void test_atomic(void) {
+ ui = __atomic_fetch_min(, 5, __ATOMIC_RELAXED); // CHECK: atomicrmw umin {{.*}} monotonic
+ si = __atomic_fetch_min(, 5, __ATOMIC_SEQ_CST); // CHECK: atomicrmw min {{.*}} seq_cst
+ ui = __atomic_fetch_max(, 5, __ATOMIC_ACQUIRE); // CHECK: atomicrmw umax {{.*}} acquire
+ si = __atomic_fetch_max(, 5, __ATOMIC_RELEASE); // CHECK: atomicrmw max {{.*}} release
+}
Index: cfe/trunk/test/Sema/atomic-ops.c
===
--- cfe/trunk/test/Sema/atomic-ops.c
+++ cfe/trunk/test/Sema/atomic-ops.c
@@ -173,6 +173,9 @@
__atomic_fetch_sub(P, 3, memory_order_seq_cst);
__atomic_fetch_sub(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
__atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
+ __atomic_fetch_min(D, 3, memory_order_seq_cst); //
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena added a comment. In https://reviews.llvm.org/D46386#1096833, @rjmccall wrote: > The actual semantic parts of the diff seem to have disappeared from the patch > posted to Phabricator, for what it's worth. It is not disappeared by itself, I removed it. I understood that you don't see any added value in the entire memory model description inside. Thank you. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that works for me. The actual semantic parts of the diff seem to have disappeared from the patch posted to Phabricator, for what it's worth. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena updated this revision to Diff 146462. delena added a comment. Added a line about *load-store* semantics of these two intrinsics. Removed the common description of memory modeling. Repository: rC Clang https://reviews.llvm.org/D46386 Files: LanguageExtensions.rst Index: LanguageExtensions.rst === --- LanguageExtensions.rst +++ LanguageExtensions.rst @@ -1975,6 +1975,32 @@ Support for constant expression evaluation for the above builtins be detected with ``__has_feature(cxx_constexpr_string_builtins)``. +Atomic Min/Max builtins with memory ordering + + +There are two atomic builtins with min/max in-memory comparison and swap. +The syntax and semantics are similar to GCC-compatible __atomic_* builtins. + +* ``__atomic_fetch_min`` +* ``__atomic_fetch_max`` + +The builtins work with signed and unsigned integers and require to specify memory ordering. +The return value is the original value that was stored in memory before comparison. + +Example: + +.. code-block:: c + + unsigned int val = __atomic_fetch_min(unsigned int *pi, unsigned int ui, __ATOMIC_RELAXED); + +The third argument is one of the memory ordering specifiers ``__ATOMIC_RELAXED``, +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + +In terms or aquire-release ordering barriers these two operations are always +considered as operations with *load-store* semantics, even when the original value +is not actually modified after comparison. + .. _langext-__c11_atomic: __c11_atomic builtins @@ -2734,4 +2760,3 @@ The ``#pragma comment(lib, ...)`` directive is supported on all ELF targets. The second parameter is the library name (without the traditional Unix prefix of ``lib``). This allows you to provide an implicit link of dependent libraries. - Index: LanguageExtensions.rst === --- LanguageExtensions.rst +++ LanguageExtensions.rst @@ -1975,6 +1975,32 @@ Support for constant expression evaluation for the above builtins be detected with ``__has_feature(cxx_constexpr_string_builtins)``. +Atomic Min/Max builtins with memory ordering + + +There are two atomic builtins with min/max in-memory comparison and swap. +The syntax and semantics are similar to GCC-compatible __atomic_* builtins. + +* ``__atomic_fetch_min`` +* ``__atomic_fetch_max`` + +The builtins work with signed and unsigned integers and require to specify memory ordering. +The return value is the original value that was stored in memory before comparison. + +Example: + +.. code-block:: c + + unsigned int val = __atomic_fetch_min(unsigned int *pi, unsigned int ui, __ATOMIC_RELAXED); + +The third argument is one of the memory ordering specifiers ``__ATOMIC_RELAXED``, +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + +In terms or aquire-release ordering barriers these two operations are always +considered as operations with *load-store* semantics, even when the original value +is not actually modified after comparison. + .. _langext-__c11_atomic: __c11_atomic builtins @@ -2734,4 +2760,3 @@ The ``#pragma comment(lib, ...)`` directive is supported on all ELF targets. The second parameter is the library name (without the traditional Unix prefix of ``lib``). This allows you to provide an implicit link of dependent libraries. - ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + rjmccall wrote: > Thank you for adding this documentation. Please do clarify what the memory > ordering semantics actually are when the atomic object does not need to be > updated, though, and verify that target code generation actually obeys that > ordering. For example, if the memory ordering makes this a release > operation, `__atomic_fetch_min` must always store the result back to the > atomic object, even if the new value was actually greater than the stored > value; I believe that would not be required with a relaxed operation. Okay, that's not what I was asking for. It's okay to assume that people understand the basic memory orderings; you don't need to copy/paste generic descriptions of them here. There is something special about min/max vs. the rest of these atomic update operations, however, which is that min and max don't always change the value of the variable. (Technically this is true of corner cases of many of the other operations — for example, you could atomically add 0 to a variable or multiply a variable by 1 — but the basic operation of min/max makes this corner case a lot more important.) I am just asking you to state definitively whether the atomic operation is still appropriately ordered if the object's value does not change. If you don't understand what I'm getting at here, maybe you should just add the relaxed ordering for now. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena updated this revision to Diff 146080.
delena added a comment.
Given more clarification about memory model of atomic operations.
Repository:
rC Clang
https://reviews.llvm.org/D46386
Files:
docs/LanguageExtensions.rst
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/Expr.cpp
lib/CodeGen/CGAtomic.cpp
lib/Sema/SemaChecking.cpp
test/CodeGen/Atomics.c
test/Sema/atomic-ops.c
Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -173,6 +173,9 @@
__atomic_fetch_sub(P, 3, memory_order_seq_cst);
__atomic_fetch_sub(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
__atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
+ __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to signed or unsigned 32-bit integer}}
+ __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be a pointer to signed or unsigned 32-bit integer}}
+ __atomic_fetch_max(p, 3); // expected-error {{too few arguments to function call, expected 3, have 2}}
__c11_atomic_fetch_and(i, 1, memory_order_seq_cst);
__c11_atomic_fetch_and(p, 1, memory_order_seq_cst); // expected-error {{must be a pointer to atomic integer}}
@@ -456,6 +459,20 @@
(void)__atomic_fetch_nand(p, val, memory_order_acq_rel);
(void)__atomic_fetch_nand(p, val, memory_order_seq_cst);
+ (void)__atomic_fetch_min(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_min(p, val, memory_order_acquire);
+ (void)__atomic_fetch_min(p, val, memory_order_consume);
+ (void)__atomic_fetch_min(p, val, memory_order_release);
+ (void)__atomic_fetch_min(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_min(p, val, memory_order_seq_cst);
+
+ (void)__atomic_fetch_max(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_max(p, val, memory_order_acquire);
+ (void)__atomic_fetch_max(p, val, memory_order_consume);
+ (void)__atomic_fetch_max(p, val, memory_order_release);
+ (void)__atomic_fetch_max(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_max(p, val, memory_order_seq_cst);
+
(void)__atomic_and_fetch(p, val, memory_order_relaxed);
(void)__atomic_and_fetch(p, val, memory_order_acquire);
(void)__atomic_and_fetch(p, val, memory_order_consume);
Index: test/CodeGen/Atomics.c
===
--- test/CodeGen/Atomics.c
+++ test/CodeGen/Atomics.c
@@ -291,3 +291,10 @@
__sync_lock_release (); // CHECK: store atomic {{.*}} release, align 8
__sync_lock_release (); // CHECK: store atomic {{.*}} release, align 8
}
+
+void test_atomic(void) {
+ ui = __atomic_fetch_min(, 5, __ATOMIC_RELAXED); // CHECK: atomicrmw umin {{.*}} monotonic
+ si = __atomic_fetch_min(, 5, __ATOMIC_SEQ_CST); // CHECK: atomicrmw min {{.*}} seq_cst
+ ui = __atomic_fetch_max(, 5, __ATOMIC_ACQUIRE); // CHECK: atomicrmw umax {{.*}} acquire
+ si = __atomic_fetch_max(, 5, __ATOMIC_RELEASE); // CHECK: atomicrmw max {{.*}} release
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3037,6 +3037,7 @@
Op == AtomicExpr::AO__atomic_exchange_n ||
Op == AtomicExpr::AO__atomic_compare_exchange_n;
bool IsAddSub = false;
+ bool IsMinMax = false;
switch (Op) {
case AtomicExpr::AO__c11_atomic_init:
@@ -3090,6 +3091,12 @@
Form = Arithmetic;
break;
+ case AtomicExpr::AO__atomic_fetch_min:
+ case AtomicExpr::AO__atomic_fetch_max:
+IsMinMax = true;
+Form = Arithmetic;
+break;
+
case AtomicExpr::AO__c11_atomic_exchange:
case AtomicExpr::AO__opencl_atomic_exchange:
case AtomicExpr::AO__atomic_exchange_n:
@@ -3172,12 +3179,21 @@
// For an arithmetic operation, the implied arithmetic must be well-formed.
if (Form == Arithmetic) {
// gcc does not enforce these rules for GNU atomics, but we do so for sanity.
-if (IsAddSub && !ValType->isIntegerType() && !ValType->isPointerType()) {
+if (IsAddSub && !ValType->isIntegerType()
+&& !ValType->isPointerType()) {
Diag(DRE->getLocStart(), diag::err_atomic_op_needs_atomic_int_or_ptr)
<< IsC11 << Ptr->getType() << Ptr->getSourceRange();
return ExprError();
}
-if (!IsAddSub && !ValType->isIntegerType()) {
+if (IsMinMax) {
+ const BuiltinType *BT = ValType->getAs();
+ if (!BT || (BT->getKind() != BuiltinType::Int &&
+ BT->getKind() != BuiltinType::UInt)) {
+Diag(DRE->getLocStart(), diag::err_atomic_op_needs_int32_or_ptr);
+return ExprError();
+ }
+}
+if (!IsAddSub && !IsMinMax && !ValType->isIntegerType()) {
Diag(DRE->getLocStart(),
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + Thank you for adding this documentation. Please do clarify what the memory ordering semantics actually are when the atomic object does not need to be updated, though, and verify that target code generation actually obeys that ordering. For example, if the memory ordering makes this a release operation, `__atomic_fetch_min` must always store the result back to the atomic object, even if the new value was actually greater than the stored value; I believe that would not be required with a relaxed operation. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena updated this revision to Diff 145646.
delena added a comment.
Removed the unsigned version of atomics. Enhanced semantics check.
Added more tests.
Added documentation.
Repository:
rC Clang
https://reviews.llvm.org/D46386
Files:
docs/LanguageExtensions.rst
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/Expr.cpp
lib/CodeGen/CGAtomic.cpp
lib/Sema/SemaChecking.cpp
test/CodeGen/Atomics.c
test/Sema/atomic-ops.c
Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -173,6 +173,9 @@
__atomic_fetch_sub(P, 3, memory_order_seq_cst);
__atomic_fetch_sub(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
__atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
+ __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to signed or unsigned 32-bit integer}}
+ __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be a pointer to signed or unsigned 32-bit integer}}
+ __atomic_fetch_max(p, 3); // expected-error {{too few arguments to function call, expected 3, have 2}}
__c11_atomic_fetch_and(i, 1, memory_order_seq_cst);
__c11_atomic_fetch_and(p, 1, memory_order_seq_cst); // expected-error {{must be a pointer to atomic integer}}
@@ -456,6 +459,20 @@
(void)__atomic_fetch_nand(p, val, memory_order_acq_rel);
(void)__atomic_fetch_nand(p, val, memory_order_seq_cst);
+ (void)__atomic_fetch_min(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_min(p, val, memory_order_acquire);
+ (void)__atomic_fetch_min(p, val, memory_order_consume);
+ (void)__atomic_fetch_min(p, val, memory_order_release);
+ (void)__atomic_fetch_min(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_min(p, val, memory_order_seq_cst);
+
+ (void)__atomic_fetch_max(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_max(p, val, memory_order_acquire);
+ (void)__atomic_fetch_max(p, val, memory_order_consume);
+ (void)__atomic_fetch_max(p, val, memory_order_release);
+ (void)__atomic_fetch_max(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_max(p, val, memory_order_seq_cst);
+
(void)__atomic_and_fetch(p, val, memory_order_relaxed);
(void)__atomic_and_fetch(p, val, memory_order_acquire);
(void)__atomic_and_fetch(p, val, memory_order_consume);
Index: test/CodeGen/Atomics.c
===
--- test/CodeGen/Atomics.c
+++ test/CodeGen/Atomics.c
@@ -291,3 +291,10 @@
__sync_lock_release (); // CHECK: store atomic {{.*}} release, align 8
__sync_lock_release (); // CHECK: store atomic {{.*}} release, align 8
}
+
+void test_atomic(void) {
+ ui = __atomic_fetch_min(, 5, __ATOMIC_RELAXED); // CHECK: atomicrmw umin {{.*}} monotonic
+ si = __atomic_fetch_min(, 5, __ATOMIC_SEQ_CST); // CHECK: atomicrmw min {{.*}} seq_cst
+ ui = __atomic_fetch_max(, 5, __ATOMIC_ACQUIRE); // CHECK: atomicrmw umax {{.*}} acquire
+ si = __atomic_fetch_max(, 5, __ATOMIC_RELEASE); // CHECK: atomicrmw max {{.*}} release
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3037,6 +3037,7 @@
Op == AtomicExpr::AO__atomic_exchange_n ||
Op == AtomicExpr::AO__atomic_compare_exchange_n;
bool IsAddSub = false;
+ bool IsMinMax = false;
switch (Op) {
case AtomicExpr::AO__c11_atomic_init:
@@ -3090,6 +3091,12 @@
Form = Arithmetic;
break;
+ case AtomicExpr::AO__atomic_fetch_min:
+ case AtomicExpr::AO__atomic_fetch_max:
+IsMinMax = true;
+Form = Arithmetic;
+break;
+
case AtomicExpr::AO__c11_atomic_exchange:
case AtomicExpr::AO__opencl_atomic_exchange:
case AtomicExpr::AO__atomic_exchange_n:
@@ -3172,12 +3179,21 @@
// For an arithmetic operation, the implied arithmetic must be well-formed.
if (Form == Arithmetic) {
// gcc does not enforce these rules for GNU atomics, but we do so for sanity.
-if (IsAddSub && !ValType->isIntegerType() && !ValType->isPointerType()) {
+if (IsAddSub && !ValType->isIntegerType()
+&& !ValType->isPointerType()) {
Diag(DRE->getLocStart(), diag::err_atomic_op_needs_atomic_int_or_ptr)
<< IsC11 << Ptr->getType() << Ptr->getSourceRange();
return ExprError();
}
-if (!IsAddSub && !ValType->isIntegerType()) {
+if (IsMinMax) {
+ const BuiltinType *BT = ValType->getAs();
+ if (!BT || (BT->getKind() != BuiltinType::Int &&
+ BT->getKind() != BuiltinType::UInt)) {
+Diag(DRE->getLocStart(), diag::err_atomic_op_needs_int32_or_ptr);
+return ExprError();
+ }
+}
+if (!IsAddSub && !IsMinMax && !ValType->isIntegerType()) {
Diag(DRE->getLocStart(),
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena added a comment. In https://reviews.llvm.org/D46386#1087533, @Anastasia wrote: > Is this some sort of a vendor extension then? OpenCL 1.2 atomic builtins > don't have ordering parameter. OpenCL 1.2 atomic builtins have relaxed semantics. Always, it is not parameter, it is defined behavior. I want to translate them to atomicrmw instruction and use one of clang intrinsics for this. I can't use _sync_fetch_*, due to the different semantics. The __atomic_* allow to specify semantics, but min/max is missing in this set. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
Anastasia added a comment. Is this some sort of a vendor extension then? OpenCL 1.2 atomic builtins don't have ordering parameter. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall added a comment. Given that these are overloaded builtins, why are there separate builtins for `min` and `umin`? If this is a Clang extension, it needs to be documented in our extensions documentation. The documentation should describe the exact ordering semantics, to the extent we want to guarantee them. For example, does `__atomic_fetch_max` order this operation with later operations even if the new value is in fact less than the current value of the variable, or does it use some treatment more like the failure case of a compare-exchange? The documentation should describe the set of allowed types. If the builtins work on pointer types, the documentation should describe the semantics of the pointer comparison; for example, is it undefined behavior if an ordinary pointer comparison would be? Also, your test case should actually check the well-formedness conditions more completely, e.g. verifying that the value type is convertible to the atomic type. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena added inline comments. Comment at: lib/Sema/SemaChecking.cpp:3098 + case AtomicExpr::AO__atomic_fetch_umax: +IsMinMax = true; +Form = Arithmetic; jfb wrote: > Should `__sync_fetch_and_min` and others also set `IsMinMax`? __sync_fetch_and_min is not variadic and not overloaded. The types of arguments are defined with the builtin itself in the def file. BUILTIN(__sync_fetch_and_min, "iiD*i", "n"). So it is checked automatically. The other __sync_fetch* functions are overloaded and checked in SemaBuiltinAtomicOverloaded() Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
jfb added inline comments. Comment at: lib/Sema/SemaChecking.cpp:3098 + case AtomicExpr::AO__atomic_fetch_umax: +IsMinMax = true; +Form = Arithmetic; Should `__sync_fetch_and_min` and others also set `IsMinMax`? Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
delena created this revision.
delena added reviewers: igorb, t.p.northover, ABataev, jfb, rjmccall.
Herald added subscribers: cfe-commits, Anastasia.
Added __atomic_fetch_min, max, umin, umax intrinsics to clang.
These intrinsics work exactly as all other __atomic_fetch_* intrinsics and
allow to create *atomicrmw* with ordering.
The similar set __sync_fetch_and_min* sets the sequentially-consistent
ordering.
We use them for OpenCL 1.2, which supports atomic operations with "relaxed"
ordering.
Repository:
rC Clang
https://reviews.llvm.org/D46386
Files:
include/clang/Basic/Builtins.def
lib/AST/Expr.cpp
lib/CodeGen/CGAtomic.cpp
lib/Sema/SemaChecking.cpp
test/Sema/atomic-ops.c
Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -173,6 +173,7 @@
__atomic_fetch_sub(P, 3, memory_order_seq_cst);
__atomic_fetch_sub(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
__atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
+ __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
__c11_atomic_fetch_and(i, 1, memory_order_seq_cst);
__c11_atomic_fetch_and(p, 1, memory_order_seq_cst); // expected-error {{must be a pointer to atomic integer}}
@@ -456,6 +457,34 @@
(void)__atomic_fetch_nand(p, val, memory_order_acq_rel);
(void)__atomic_fetch_nand(p, val, memory_order_seq_cst);
+ (void)__atomic_fetch_min(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_min(p, val, memory_order_acquire);
+ (void)__atomic_fetch_min(p, val, memory_order_consume);
+ (void)__atomic_fetch_min(p, val, memory_order_release);
+ (void)__atomic_fetch_min(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_min(p, val, memory_order_seq_cst);
+
+ (void)__atomic_fetch_max(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_max(p, val, memory_order_acquire);
+ (void)__atomic_fetch_max(p, val, memory_order_consume);
+ (void)__atomic_fetch_max(p, val, memory_order_release);
+ (void)__atomic_fetch_max(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_max(p, val, memory_order_seq_cst);
+
+ (void)__atomic_fetch_umin(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_umin(p, val, memory_order_acquire);
+ (void)__atomic_fetch_umin(p, val, memory_order_consume);
+ (void)__atomic_fetch_umin(p, val, memory_order_release);
+ (void)__atomic_fetch_umin(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_umin(p, val, memory_order_seq_cst);
+
+ (void)__atomic_fetch_umax(p, val, memory_order_relaxed);
+ (void)__atomic_fetch_umax(p, val, memory_order_acquire);
+ (void)__atomic_fetch_umax(p, val, memory_order_consume);
+ (void)__atomic_fetch_umax(p, val, memory_order_release);
+ (void)__atomic_fetch_umax(p, val, memory_order_acq_rel);
+ (void)__atomic_fetch_umax(p, val, memory_order_seq_cst);
+
(void)__atomic_and_fetch(p, val, memory_order_relaxed);
(void)__atomic_and_fetch(p, val, memory_order_acquire);
(void)__atomic_and_fetch(p, val, memory_order_consume);
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3037,6 +3037,7 @@
Op == AtomicExpr::AO__atomic_exchange_n ||
Op == AtomicExpr::AO__atomic_compare_exchange_n;
bool IsAddSub = false;
+ bool IsMinMax = false;
switch (Op) {
case AtomicExpr::AO__c11_atomic_init:
@@ -3090,6 +3091,14 @@
Form = Arithmetic;
break;
+ case AtomicExpr::AO__atomic_fetch_min:
+ case AtomicExpr::AO__atomic_fetch_max:
+ case AtomicExpr::AO__atomic_fetch_umin:
+ case AtomicExpr::AO__atomic_fetch_umax:
+IsMinMax = true;
+Form = Arithmetic;
+break;
+
case AtomicExpr::AO__c11_atomic_exchange:
case AtomicExpr::AO__opencl_atomic_exchange:
case AtomicExpr::AO__atomic_exchange_n:
@@ -3172,12 +3181,13 @@
// For an arithmetic operation, the implied arithmetic must be well-formed.
if (Form == Arithmetic) {
// gcc does not enforce these rules for GNU atomics, but we do so for sanity.
-if (IsAddSub && !ValType->isIntegerType() && !ValType->isPointerType()) {
+if ((IsAddSub || IsMinMax) && !ValType->isIntegerType()
+&& !ValType->isPointerType()) {
Diag(DRE->getLocStart(), diag::err_atomic_op_needs_atomic_int_or_ptr)
<< IsC11 << Ptr->getType() << Ptr->getSourceRange();
return ExprError();
}
-if (!IsAddSub && !ValType->isIntegerType()) {
+if (!IsAddSub && !IsMinMax && !ValType->isIntegerType()) {
Diag(DRE->getLocStart(), diag::err_atomic_op_bitwise_needs_atomic_int)
<< IsC11 << Ptr->getType() << Ptr->getSourceRange();
return ExprError();
Index: lib/CodeGen/CGAtomic.cpp
===
---
