[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.
Closed by commit rL310082: Add OpenCL 2.0 atomic builtin functions as Clang
builtin (authored by yaxunl).
Changed prior to commit:
https://reviews.llvm.org/D28691?vs=109612=109782#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28691
Files:
cfe/trunk/docs/LanguageExtensions.rst
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/Basic/Builtins.def
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Basic/SyncScope.h
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/AST/StmtPrinter.cpp
cfe/trunk/lib/Basic/Targets/AMDGPU.cpp
cfe/trunk/lib/CodeGen/CGAtomic.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/TargetInfo.cpp
cfe/trunk/lib/CodeGen/TargetInfo.h
cfe/trunk/lib/Frontend/InitPreprocessor.cpp
cfe/trunk/lib/Headers/opencl-c.h
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/CodeGenOpenCL/atomic-ops-libcall.cl
cfe/trunk/test/CodeGenOpenCL/atomic-ops.cl
cfe/trunk/test/Preprocessor/init.c
cfe/trunk/test/Preprocessor/predefined-macros.c
cfe/trunk/test/SemaOpenCL/atomic-ops.cl
Index: cfe/trunk/lib/CodeGen/TargetInfo.h
===
--- cfe/trunk/lib/CodeGen/TargetInfo.h
+++ cfe/trunk/lib/CodeGen/TargetInfo.h
@@ -19,6 +19,7 @@
#include "CGValue.h"
#include "clang/AST/Type.h"
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SyncScope.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
@@ -260,6 +261,10 @@
virtual llvm::Constant *
performAddrSpaceCast(CodeGenModule , llvm::Constant *V, unsigned SrcAddr,
unsigned DestAddr, llvm::Type *DestTy) const;
+
+ /// Get the syncscope used in LLVM IR.
+ virtual llvm::SyncScope::ID getLLVMSyncScopeID(SyncScope S,
+ llvm::LLVMContext ) const;
};
} // namespace CodeGen
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -444,6 +444,11 @@
return llvm::ConstantExpr::getPointerCast(Src, DestTy);
}
+llvm::SyncScope::ID
+TargetCodeGenInfo::getLLVMSyncScopeID(SyncScope S, llvm::LLVMContext ) const {
+ return C.getOrInsertSyncScopeID(""); /* default sync scope */
+}
+
static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays);
/// isEmptyField - Return true iff a the field is "empty", that is it
@@ -7430,6 +7435,8 @@
}
unsigned getGlobalVarAddressSpace(CodeGenModule ,
const VarDecl *D) const override;
+ llvm::SyncScope::ID getLLVMSyncScopeID(SyncScope S,
+ llvm::LLVMContext ) const override;
};
}
@@ -7539,6 +7546,26 @@
return DefaultGlobalAS;
}
+llvm::SyncScope::ID
+AMDGPUTargetCodeGenInfo::getLLVMSyncScopeID(SyncScope S,
+llvm::LLVMContext ) const {
+ StringRef Name;
+ switch (S) {
+ case SyncScope::OpenCLWorkGroup:
+Name = "workgroup";
+break;
+ case SyncScope::OpenCLDevice:
+Name = "agent";
+break;
+ case SyncScope::OpenCLAllSVMDevices:
+Name = "";
+break;
+ case SyncScope::OpenCLSubGroup:
+Name = "subgroup";
+ }
+ return C.getOrInsertSyncScopeID(Name);
+}
+
//===--===//
// SPARC v8 ABI Implementation.
// Based on the SPARC Compliance Definition version 2.4.1.
Index: cfe/trunk/lib/CodeGen/CGExpr.cpp
===
--- cfe/trunk/lib/CodeGen/CGExpr.cpp
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp
@@ -48,7 +48,7 @@
llvm::Value *CodeGenFunction::EmitCastToVoidPtr(llvm::Value *value) {
unsigned addressSpace =
-cast(value->getType())->getAddressSpace();
+ cast(value->getType())->getAddressSpace();
llvm::PointerType *destType = Int8PtrTy;
if (addressSpace)
Index: cfe/trunk/lib/CodeGen/CGAtomic.cpp
===
--- cfe/trunk/lib/CodeGen/CGAtomic.cpp
+++ cfe/trunk/lib/CodeGen/CGAtomic.cpp
@@ -15,6 +15,7 @@
#include "CGRecordLayout.h"
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
+#include "TargetInfo.h"
#include "clang/AST/ASTContext.h"
#include "clang/CodeGen/CGFunctionInfo.h"
#include "llvm/IR/DataLayout.h"
@@ -359,13 +360,15 @@
Address Val1, Address Val2,
uint64_t Size,
llvm::AtomicOrdering SuccessOrder,
- llvm::AtomicOrdering FailureOrder) {
+ llvm::AtomicOrdering FailureOrder,
+ llvm::SyncScope::ID Scope) {
// Note that cmpxchg doesn't support weak cmpxchg, at least at
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Still LGTM. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked 3 inline comments as done.
yaxunl added a comment.
Ping. Any other comments? Thanks.
Comment at: lib/AST/Expr.cpp:4000-4004
+ if (auto AT = T->getAs()) {
+return AT->getValueType();
+ } else {
+return T;
+ }
bader wrote:
> No need in else branch after return:
> ```
> if (...) {
> return AT->getValueType();
> }
>
> return T;
> ```
>
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
will fix when committing.
Comment at: test/SemaOpenCL/atomic-ops.cl:1
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify
-fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify
-fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
bader wrote:
> It's a pity, we have to parse the whole opencl-c.h file to get two enums and
> one typedef...
Since there is change in opencl-c.h, using the header can test that.
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
bader accepted this revision.
bader added inline comments.
Comment at: lib/AST/Expr.cpp:4000-4004
+ if (auto AT = T->getAs()) {
+return AT->getValueType();
+ } else {
+return T;
+ }
No need in else branch after return:
```
if (...) {
return AT->getValueType();
}
return T;
```
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
Comment at: test/SemaOpenCL/atomic-ops.cl:1
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify
-fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify
-fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
It's a pity, we have to parse the whole opencl-c.h file to get two enums and
one typedef...
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked 10 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/SyncScope.h:21
+/// \brief Defines the synch scope values used by the atomic builtins and
+/// expressions
+enum class SyncScope {
rjmccall wrote:
> If the numeric values here were chosen to align with the arguments to some
> runtime function, that's important to leave as a comment.
I have added the comments and updated the review.. On the Phabricator web page,
it may take a bit effort to find it since it is interleaved with reviewers'
comments.
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
t-tye accepted this revision. t-tye added a comment. LGTM https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 109612.
yaxunl marked an inline comment as done.
yaxunl added a comment.
Added documentation about __OPENCL_MEMORY_SCOPE_* by Tony's comments.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Frontend/InitPreprocessor.cpp
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/Preprocessor/init.c
test/Preprocessor/predefined-macros.c
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,161 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: docs/LanguageExtensions.rst:1935 +builtin function, and are named with a ``__opencl_`` prefix.) Low-level ARM exclusive memory builtins t-tye wrote: > Should it also say: > > ``` > The macros ``__OPENCL_MEMORY_SCOPE_WORK_ITEM``, > ``__OPENCL_MEMORY_SCOPE_WORK_GROUP``, ``__OPENCL_MEMORY_SCOPE_DEVICE``, > ``__OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES``, and > ``__OPENCL_MEMORY_SCOPE_SUB_GROUP`` are provided, with values corresponding > to the enumerators of OpenCL's ``memory_scope`` enumeration. > ``` Thanks. Will do. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
t-tye added inline comments. Comment at: docs/LanguageExtensions.rst:1935 +builtin function, and are named with a ``__opencl_`` prefix.) Low-level ARM exclusive memory builtins Should it also say: ``` The macros ``__OPENCL_MEMORY_SCOPE_WORK_ITEM``, ``__OPENCL_MEMORY_SCOPE_WORK_GROUP``, ``__OPENCL_MEMORY_SCOPE_DEVICE``, ``__OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES``, and ``__OPENCL_MEMORY_SCOPE_SUB_GROUP`` are provided, with values corresponding to the enumerators of OpenCL's ``memory_scope`` enumeration. ``` https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 109591.
yaxunl added a comment.
Add comments to SyncScope.h
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Frontend/InitPreprocessor.cpp
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/Preprocessor/init.c
test/Preprocessor/predefined-macros.c
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,161 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment.
LGTM. I'm fine with the plan to handle potentially non-constant scopes in a
follow-up patch.
Comment at: include/clang/Basic/SyncScope.h:21
+/// \brief Defines the synch scope values used by the atomic builtins and
+/// expressions
+enum class SyncScope {
If the numeric values here were chosen to align with the arguments to some
runtime function, that's important to leave as a comment.
Comment at: lib/Headers/opencl-c.h:13145-13150
memory_scope_work_item,
memory_scope_work_group,
memory_scope_device,
memory_scope_all_svm_devices,
+#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
memory_scope_sub_group
yaxunl wrote:
> t-tye wrote:
> > Do these have to have the same values as the SycnScope enumeration? Should
> > that be ensured in a similar way to the memory_order enumeration?
> It is desirable to have the same value as SyncScope enumeration, otherwise
> the library has to do the translation when calling __opencl_atomic_* builtins.
>
> Will do.
Since we're defining these builtins ourselves de novo, it's fine to pick
argument values that align with what the existing runtime functions expect.
Once the builtins are defined and in-use, of course, we cannot subsequently
change the builtin values, even if the runtime changes.
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 109556.
yaxunl added a comment.
Add assert to make sure pre-defined macros __OPENCL_MEMORY_SCOP_* to be
consistent with SyncScope enum.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Frontend/InitPreprocessor.cpp
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/Preprocessor/init.c
test/Preprocessor/predefined-macros.c
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,161 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 109542.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.
Herald added subscribers: aheejin, dschuff, jfb.
Revised by Tony's and John's comments.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Frontend/InitPreprocessor.cpp
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/Preprocessor/init.c
test/Preprocessor/predefined-macros.c
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,161 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_group);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/Builtins.def:717
+ATOMIC_BUILTIN(__opencl_atomic_fetch_max, "v.", "t")
+
#undef ATOMIC_BUILTIN
t-tye wrote:
> Will the OpenCL 2.0 memory fences also be supported which also have a memory
> order and memory scope?
I am considering supporting it with a separate patch since this patch is
already quite large.
Comment at: include/clang/Basic/SyncScope.h:23
+enum class SyncScope {
+ OpenCLWorkItem = 0,
+ OpenCLWorkGroup = 1,
t-tye wrote:
> The OpenCL workitem scope is only used for image fences and does not apply to
> atomic operations so not sure that it should be in this enumeration which is
> used only for memory atomics.
You are right. I think we should drop it from this enum for now.
Comment at: lib/CodeGen/CGAtomic.cpp:896
+return V;
+ auto DestAS = getContext().getTargetAddressSpace(LangAS::opencl_generic);
+ auto T = V->getType();
rjmccall wrote:
> You can sink this line and the next.
will do.
Comment at: lib/CodeGen/CGCall.cpp:3911
+ V = Builder.CreateBitOrPointerCast(V,
+ IRFuncTy->getParamType(FirstIRArg));
rjmccall wrote:
> No. Callers should ensure that they've added the right argument type, at
> least at the level of address spaces.
Will remove.
Comment at: lib/CodeGen/TargetInfo.cpp:7554-7555
+ switch (S) {
+ case SyncScope::OpenCLWorkItem:
+Name = "singlethread";
+break;
t-tye wrote:
> OpenCL only uses workitem for image fences which are not the same as atomic
> memory fences.
>
> For image fences I don't think it would map to singlethread which is intended
> for signal handler support to ensure a signal handler has visibility of the
> updates done by a thread which is more of an optimization barrier. In
> contrast an image fence may need to flush caches to make the image and vector
> access paths coherent in a single thread.
>
> Since this patch does not support fences probably want to leave workitem
> scope out. Current AMDGPU targets do not need to do anything for an OpenCL
> image fence, but other targets may need to generate an intrinsic since there
> is no LLVMIR instruction for this.
Thanks. I will remove this for now.
Comment at: lib/Headers/opencl-c.h:13145-13150
memory_scope_work_item,
memory_scope_work_group,
memory_scope_device,
memory_scope_all_svm_devices,
+#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
memory_scope_sub_group
t-tye wrote:
> Do these have to have the same values as the SycnScope enumeration? Should
> that be ensured in a similar way to the memory_order enumeration?
It is desirable to have the same value as SyncScope enumeration, otherwise the
library has to do the translation when calling __opencl_atomic_* builtins.
Will do.
Comment at: lib/Sema/SemaChecking.cpp:3160
+Op == AtomicExpr::AO__opencl_atomic_load)
+? 0
+: 1);
Anastasia wrote:
> Could we merge this and the line after please.
will do
Comment at: lib/Sema/SemaChecking.cpp:3103
+Diag(Scope->getLocStart(),
+ diag::err_atomic_op_has_non_constant_synch_scope)
+<< Scope->getSourceRange();
rjmccall wrote:
> t-tye wrote:
> > IIRC OpenCL allows the scope to be a runtime value. So will doing this will
> > likely cause failures in conformance?
> Ah, if that's true, you'll need to emit a switch in IRGen, the same way we
> handle non-constant memory orders.
Will support it in another patch since this one is already quite large.
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment. The patch generally looks good, but if you need to handle non-constant scopes, you should submit a new patch to address that. Comment at: lib/CodeGen/CGAtomic.cpp:896 +return V; + auto DestAS = getContext().getTargetAddressSpace(LangAS::opencl_generic); + auto T = V->getType(); You can sink this line and the next. Comment at: lib/Sema/SemaChecking.cpp:3103 +Diag(Scope->getLocStart(), + diag::err_atomic_op_has_non_constant_synch_scope) +<< Scope->getSourceRange(); t-tye wrote: > IIRC OpenCL allows the scope to be a runtime value. So will doing this will > likely cause failures in conformance? Ah, if that's true, you'll need to emit a switch in IRGen, the same way we handle non-constant memory orders. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
t-tye added inline comments.
Comment at: include/clang/Basic/SyncScope.h:23
+enum class SyncScope {
+ OpenCLWorkItem = 0,
+ OpenCLWorkGroup = 1,
The OpenCL workitem scope is only used for image fences and does not apply to
atomic operations so not sure that it should be in this enumeration which is
used only for memory atomics.
Comment at: lib/CodeGen/TargetInfo.cpp:7554-7555
+ switch (S) {
+ case SyncScope::OpenCLWorkItem:
+Name = "singlethread";
+break;
OpenCL only uses workitem for image fences which are not the same as atomic
memory fences.
For image fences I don't think it would map to singlethread which is intended
for signal handler support to ensure a signal handler has visibility of the
updates done by a thread which is more of an optimization barrier. In contrast
an image fence may need to flush caches to make the image and vector access
paths coherent in a single thread.
Since this patch does not support fences probably want to leave workitem scope
out. Current AMDGPU targets do not need to do anything for an OpenCL image
fence, but other targets may need to generate an intrinsic since there is no
LLVMIR instruction for this.
Comment at: lib/Headers/opencl-c.h:13145-13150
memory_scope_work_item,
memory_scope_work_group,
memory_scope_device,
memory_scope_all_svm_devices,
+#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
memory_scope_sub_group
Do these have to have the same values as the SycnScope enumeration? Should that
be ensured in a similar way to the memory_order enumeration?
Comment at: lib/Sema/SemaChecking.cpp:3103
+Diag(Scope->getLocStart(),
+ diag::err_atomic_op_has_non_constant_synch_scope)
+<< Scope->getSourceRange();
IIRC OpenCL allows the scope to be a runtime value. So will doing this will
likely cause failures in conformance?
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 109406.
yaxunl marked 29 inline comments as done.
yaxunl added a comment.
Revised by reviewers' comments.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,156 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+ __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_item);
+
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added inline comments.
Comment at: include/clang/Basic/SyncScope.h:1
+//===--- SyncScope.h - atomic synchronization scopes *- C++
-*-===//
+//
Capitalization.
Comment at: include/clang/Basic/SyncScope.h:20
+
+namespace SyncScope {
+
LLVM uses this namespace pattern in code that predates the addition of scoped
enums to C++. Those days are behind us; we should just use a scoped enum.
Comment at: include/clang/Basic/SyncScope.h:22
+
+/// \brief Defines the synch scope values used by the atomic instructions.
+///
It defines the synch scope values used by the atomic builtins and expressions.
LLVM's headers define the values used by the instructions.
Comment at: include/clang/Basic/SyncScope.h:25-29
+ SingleThread = 0,
+ WorkGroup = 1,
+ Device= 2,
+ System= 3,
+ SubGroup = 4,
Anastasia wrote:
> t-tye wrote:
> > Since the builtins are being named as __opencl then should these also be
> > named as opencl_ since they are the memory scopes for OpenCL using the
> > OpenCL numeric values?
> >
> > If another language wants to use memory scopes, would it then add its own
> > langx_* names in a similar way that is done for address spaces where the
> > LangAS enumeration type has values for each distinct language. Each target
> > is then responsible for mapping each language scope to the appropriate
> > target specific scope as is done for address spaces?
> >
> > If so then the builtins are really supporting the concept of memory scopes
> > and are not language specific as this enumeration can support multiple
> > languages in the same way as the LangAS enumeration supports multiple
> > languages. If so the builtins would be better named to reflect this as
> > @b-sumner suggested.
> We generally prefix the names of OpenCL specific implementations. So perhaps
> we should do some renaming here in case we don't intend this to be generic
> implementation.
I agree that we should name the OpenCL-specific ones, like WorkGroup, with an
OpenCL prefix.
Comment at: include/clang/Basic/SyncScope.h:32
+
+inline unsigned getMaxValue(void) {
+ return SubGroup;
This is C++; please just use () instead of (void).
Comment at: lib/CodeGen/CGAtomic.cpp:503
+ ->getAs()
+ ->isSignedIntegerType();
+}
None of the .getTypePtr() stuff here is necessary.
This function shouldn't really be necessary. I would encourage you to add a
getValueType() accessor to AtomicExpr:
QualType AtomicExpr::getValueType() const {
auto T = getPtr()->getType()->castTo()->getPointeeType();
if (auto AT = T->getAs()) {
return AT->getValueType();
} else {
return T;
}
}
You can then just use E->getValueType()->isSignedIntegerType() and eliminate
this helper function.
Comment at: lib/CodeGen/CGAtomic.cpp:919
+->getPointeeType()
+.getAddressSpace();
+ auto *DestType = T->getPointerElementType()->getPointerTo(DestAS);
Again you're using getTypePtr() unnecessarily.
The check should be whether the AST-level address spaces match, not whether the
lowered address spaces match.
Please pass E->getType() instead of E here.
You should remove the DoIt parameter and just check E->isOpenCL() (where E is
the AtomicExpr already in scope).
Comment at: lib/CodeGen/CGCall.cpp:3911
+ V = Builder.CreateBitOrPointerCast(V,
+ IRFuncTy->getParamType(FirstIRArg));
No. Callers should ensure that they've added the right argument type, at least
at the level of address spaces.
Comment at: lib/CodeGen/TargetInfo.h:266
+ /// Get the syncscope name used in LLVM IR.
+ virtual llvm::StringRef getSyncScopeName(SyncScope::ID S) const;
};
Why does this return a StringRef instead of an llvm::SynchronizationScope?
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment. In https://reviews.llvm.org/D28691#823810, @Anastasia wrote: > In https://reviews.llvm.org/D28691#820684, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > > > > > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > > > > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > > > > > > > There are other languages for heterogeneous compute that have scopes, > > > > > although not exposed quite as explicitly as OpenCL. For example > > > > > AMD's "HC" language. And any language making use of clang and > > > > > targeting SPIR-V would likely use these builtins. I think a more > > > > > generic prefix is appropriate, and "scoped" tells us exactly when > > > > > these are needed. > > > > > > > > > > > > But would those languages use the same language design for these scopes > > > > as OpenCL if they did expose them, as opposed to some more elaborate > > > > scoping specification? My objection is not that the concept is > > > > inherently OpenCL-specific, it's that the presentation in the language > > > > might be inherently OpenCL-specific, which makes staying in the opencl > > > > namespace is prudent. > > > > > > > > > Are you envisioning a language far enough from C/C++ that a standard > > > library or header would not be able to map a scoped atomic operation into > > > a call to one of these new builtins? Would we expect more of such > > > languages than languages that would do such a mapping? > > > > > > If you're using Clang as a frontend for your language, it must be similar > > enough to C to call a builtin function. That's not at issue. The central > > question here is whether these builtins are meaningfully general outside of > > OpenCL. The concept of heterogenous computation is certainly not specific > > to OpenCL; however, these builtins are defined in terms of scopes — "work > > item", "work group", "device", and "all svm devices" — which, it seems to > > me, are basically only defined by reference to the OpenCL architecture. A > > different heterogenous compute environment might reasonably formalize > > scopes in a completely different way; for example, it might wish to be more > > explicit about exactly which peers / devices to synchronize with. > > > > SPIR is explicitly defined on top of the OpenCL model. Users should be > > able to use OpenCL builtins when targeting it. That does not make those > > builtins more general than OpenCL. > > > > John. > > > The scope concept in OpenCL is fairly generic. And the builtins just take one > extra argument on top of the existing C11 builtin style. The OpenCL scopes > have of course specific meaning to the OpenCL model, but there is nothing > preventing other uses of the scope feature. Yes, it is possible that some other language could introduce exactly the same scope-atomics concept only with a slightly different enumeration of scopes. But then we really shouldn't allow the OpenCL scopes to be used in that language, which means there would still not be a language-independent way of using these builtins. > As far as I understand atomic scope implementation in LLVM is fairly generic > wrt scope types and it is intended for broader functionality than just OpenCL. In some ways this is reasoning the wrong way around. I am not deeply informed about heterogenous computing, so I am happy to accept that llvm::SynchronizationScope is well-designed for our current needs. But LLVM's representation is, by design, ultimately just an implementation detail and can be easily updated — it's just a matter of changing a few calls and adding an upgrade path to the bitcode loader, exactly as we did when we introduced llvm::SynchronizationScope. That is not true of source language features, even builtins; their design is a contract with programmers, and the bar is substantially higher. We do not have a compelling reason to claim that these are generally useful, so we should not. They should stay namespaced and be flagged as language-specific builtins. John. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
Anastasia added a comment. In https://reviews.llvm.org/D28691#820684, @rjmccall wrote: > In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > > > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > > > > > There are other languages for heterogeneous compute that have scopes, > > > > although not exposed quite as explicitly as OpenCL. For example AMD's > > > > "HC" language. And any language making use of clang and targeting > > > > SPIR-V would likely use these builtins. I think a more generic prefix > > > > is appropriate, and "scoped" tells us exactly when these are needed. > > > > > > > > > But would those languages use the same language design for these scopes > > > as OpenCL if they did expose them, as opposed to some more elaborate > > > scoping specification? My objection is not that the concept is > > > inherently OpenCL-specific, it's that the presentation in the language > > > might be inherently OpenCL-specific, which makes staying in the opencl > > > namespace is prudent. > > > > > > Are you envisioning a language far enough from C/C++ that a standard > > library or header would not be able to map a scoped atomic operation into a > > call to one of these new builtins? Would we expect more of such languages > > than languages that would do such a mapping? > > > If you're using Clang as a frontend for your language, it must be similar > enough to C to call a builtin function. That's not at issue. The central > question here is whether these builtins are meaningfully general outside of > OpenCL. The concept of heterogenous computation is certainly not specific to > OpenCL; however, these builtins are defined in terms of scopes — "work > item", "work group", "device", and "all svm devices" — which, it seems to me, > are basically only defined by reference to the OpenCL architecture. A > different heterogenous compute environment might reasonably formalize scopes > in a completely different way; for example, it might wish to be more explicit > about exactly which peers / devices to synchronize with. > > SPIR is explicitly defined on top of the OpenCL model. Users should be able > to use OpenCL builtins when targeting it. That does not make those builtins > more general than OpenCL. > > John. The scope concept in OpenCL is fairly generic. And the builtins just take one extra argument on top of the existing C11 builtin style. The OpenCL scopes have of course specific meaning to the OpenCL model, but there is nothing preventing other uses of the scope feature. As far as I understand atomic scope implementation in LLVM is fairly generic wrt scope types and it is intended for broader functionality than just OpenCL. So I would vote for having this as generic as possible in Clang too even though I don't think name prefix `__opencl` is preventing from using this feature in other languages unless we would disallow the builtins in the other language dialects. Which is not the case with the current patch because the builtins are added as `ATOMIC_BUILTIN` and not `LANGBUILTIN`. We have for example used C11 builtin in OpenCL already. So other cases are possible too. Comment at: include/clang/Basic/SyncScope.h:25-29 + SingleThread = 0, + WorkGroup = 1, + Device= 2, + System= 3, + SubGroup = 4, t-tye wrote: > Since the builtins are being named as __opencl then should these also be > named as opencl_ since they are the memory scopes for OpenCL using the OpenCL > numeric values? > > If another language wants to use memory scopes, would it then add its own > langx_* names in a similar way that is done for address spaces where the > LangAS enumeration type has values for each distinct language. Each target is > then responsible for mapping each language scope to the appropriate target > specific scope as is done for address spaces? > > If so then the builtins are really supporting the concept of memory scopes > and are not language specific as this enumeration can support multiple > languages in the same way as the LangAS enumeration supports multiple > languages. If so the builtins would be better named to reflect this as > @b-sumner suggested. We generally prefix the names of OpenCL specific implementations. So perhaps we should do some renaming here in case we don't intend this to be generic implementation. Comment at: lib/Sema/SemaChecking.cpp:3160 +Op == AtomicExpr::AO__opencl_atomic_load) +? 0 +: 1); Could we merge this and the line after please. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
t-tye added inline comments. Comment at: include/clang/Basic/Builtins.def:717 +ATOMIC_BUILTIN(__opencl_atomic_fetch_max, "v.", "t") + #undef ATOMIC_BUILTIN Will the OpenCL 2.0 memory fences also be supported which also have a memory order and memory scope? Comment at: include/clang/Basic/SyncScope.h:25-29 + SingleThread = 0, + WorkGroup = 1, + Device= 2, + System= 3, + SubGroup = 4, Since the builtins are being named as __opencl then should these also be named as opencl_ since they are the memory scopes for OpenCL using the OpenCL numeric values? If another language wants to use memory scopes, would it then add its own langx_* names in a similar way that is done for address spaces where the LangAS enumeration type has values for each distinct language. Each target is then responsible for mapping each language scope to the appropriate target specific scope as is done for address spaces? If so then the builtins are really supporting the concept of memory scopes and are not language specific as this enumeration can support multiple languages in the same way as the LangAS enumeration supports multiple languages. If so the builtins would be better named to reflect this as @b-sumner suggested. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment. In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > > > There are other languages for heterogeneous compute that have scopes, > > > although not exposed quite as explicitly as OpenCL. For example AMD's > > > "HC" language. And any language making use of clang and targeting SPIR-V > > > would likely use these builtins. I think a more generic prefix is > > > appropriate, and "scoped" tells us exactly when these are needed. > > > > > > But would those languages use the same language design for these scopes as > > OpenCL if they did expose them, as opposed to some more elaborate scoping > > specification? My objection is not that the concept is inherently > > OpenCL-specific, it's that the presentation in the language might be > > inherently OpenCL-specific, which makes staying in the opencl namespace is > > prudent. > > > Are you envisioning a language far enough from C/C++ that a standard library > or header would not be able to map a scoped atomic operation into a call to > one of these new builtins? Would we expect more of such languages than > languages that would do such a mapping? If you're using Clang as a frontend for your language, it must be similar enough to C to call a builtin function. That's not at issue. The central question here is whether these builtins are meaningfully general outside of OpenCL. The concept of heterogenous computation is certainly not specific to OpenCL; however, these builtins are defined in terms of scopes — "work item", "work group", "device", and "all svm devices" — which, it seems to me, are basically only defined by reference to the OpenCL architecture. A different heterogenous compute environment might reasonably formalize scopes in a completely different way; for example, it might wish to be more explicit about exactly which peers / devices to synchronize with. SPIR is explicitly defined on top of the OpenCL model. Users should be able to use OpenCL builtins when targeting it. That does not make those builtins more general than OpenCL. John. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
b-sumner added a comment. In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > There are other languages for heterogeneous compute that have scopes, > > although not exposed quite as explicitly as OpenCL. For example AMD's "HC" > > language. And any language making use of clang and targeting SPIR-V would > > likely use these builtins. I think a more generic prefix is appropriate, > > and "scoped" tells us exactly when these are needed. > > > But would those languages use the same language design for these scopes as > OpenCL if they did expose them, as opposed to some more elaborate scoping > specification? My objection is not that the concept is inherently > OpenCL-specific, it's that the presentation in the language might be > inherently OpenCL-specific, which makes staying in the opencl namespace is > prudent. Are you envisioning a language far enough from C/C++ that a standard library or header would not be able to map a scoped atomic operation into a call to one of these new builtins? Would we expect more of such languages than languages that would do such a mapping? https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment. In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > There are other languages for heterogeneous compute that have scopes, > although not exposed quite as explicitly as OpenCL. For example AMD's "HC" > language. And any language making use of clang and targeting SPIR-V would > likely use these builtins. I think a more generic prefix is appropriate, and > "scoped" tells us exactly when these are needed. But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification? My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
b-sumner added a comment. In https://reviews.llvm.org/D28691#820526, @rjmccall wrote: > In https://reviews.llvm.org/D28691#820489, @yaxunl wrote: > > > In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > > > > > Can we drop the "opencl" part of the name and use something like > > > __scoped_atomic_*? Also, it may not make sense to support non-constant > > > scope here since we can't predict what other scopes may be added by other > > > languages in the future. > > > > > > we could use the approach of LangAS, i.e. we allow targets to map all > > language specific scopes to target-specific scope names, since IR only > > cares about scope names, which are target specific. And this is what the > > current implementation does. > > > > I have no objection to use the __scoped_atomic_ name. It is more general > > and extensible. John/Anastasia, any comments? Thanks. > > > I think I would prefer __opencl_atomic_* until we have some evidence that > this concept is more general than just OpenCL. There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment. In https://reviews.llvm.org/D28691#820489, @yaxunl wrote: > In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > > > Can we drop the "opencl" part of the name and use something like > > __scoped_atomic_*? Also, it may not make sense to support non-constant > > scope here since we can't predict what other scopes may be added by other > > languages in the future. > > > we could use the approach of LangAS, i.e. we allow targets to map all > language specific scopes to target-specific scope names, since IR only cares > about scope names, which are target specific. And this is what the current > implementation does. > > I have no objection to use the __scoped_atomic_ name. It is more general and > extensible. John/Anastasia, any comments? Thanks. I think I would prefer __opencl_atomic_* until we have some evidence that this concept is more general than just OpenCL. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl added a comment. In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > Can we drop the "opencl" part of the name and use something like > __scoped_atomic_*? Also, it may not make sense to support non-constant > scope here since we can't predict what other scopes may be added by other > languages in the future. we could use the approach of LangAS, i.e. we allow targets to map all language specific scopes to target-specific scope names, since IR only cares about scope names, which are target specific. And this is what the current implementation does. I have no objection to use the __scoped_atomic_ name. It is more general and extensible. John/Anastasia, any comments? Thanks. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 108127.
yaxunl added a comment.
Add min/max and missing file.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,156 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+ __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_item);
+
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked an inline comment as done. yaxunl added a comment. In https://reviews.llvm.org/D28691#820375, @kzhuravl wrote: > Seems like SyncScope.h file is missing? Right. I will add it. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
kzhuravl added a comment. Seems like SyncScope.h file is missing? https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: include/clang/Basic/Builtins.def:713 +ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t") +ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t") + b-sumner wrote: > yaxunl wrote: > > Anastasia wrote: > > > What about min/max? I believe they will need to have the scope too. > > They are not 2.0 atomic builtin functions. They can be implemented as > > library functions through 2.0 atomic builtin functions. > Yes, they are. Please look again at 6.13.11.7.5 in the 2.0 C spec. sorry I thought they are just some atomic extensions since C++11 atomic builtins do not have those. I will add them. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
b-sumner added inline comments. Comment at: include/clang/Basic/Builtins.def:713 +ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t") +ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t") + yaxunl wrote: > Anastasia wrote: > > What about min/max? I believe they will need to have the scope too. > They are not 2.0 atomic builtin functions. They can be implemented as library > functions through 2.0 atomic builtin functions. Yes, they are. Please look again at 6.13.11.7.5 in the 2.0 C spec. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 108073.
yaxunl marked 16 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a reviewer: kzhuravl.
yaxunl added a comment.
Herald added a subscriber: nhaehnle.
Rebased to ToT and revised by Anastasia's comments.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,151 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2,
+ global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
+ constant atomic_int *i_c) {
+ __opencl_atomic_init(I, 5); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function call, expected 3, have 1}}
+ __opencl_atomic_load(0, 0, 0, 0); // expected-error {{too many arguments to function call, expected 3, have 4}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int *)0, 0, 0, 0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic type ('__generic int *' invalid)}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+ __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl marked 16 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/Builtins.def:713
+ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t")
+ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t")
+
Anastasia wrote:
> What about min/max? I believe they will need to have the scope too.
They are not 2.0 atomic builtin functions. They can be implemented as library
functions through 2.0 atomic builtin functions.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6956
+ "synchronization scope argument to atomic operation is invalid">;
+def err_atomic_op_has_non_constant_synch_scope : Error<
+ "non-constant synchronization scope argument to atomic operation is not
supported">;
Anastasia wrote:
> Btw, is this disallowed by the spec? Can't find anything relevant.
Just temporarily not supported by Clang. Will add support later.
Comment at: lib/CodeGen/CGAtomic.cpp:678
RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
+ bool IsOpenCL = E->isOpenCL();
QualType AtomicTy = E->getPtr()->getType()->getPointeeType();
Anastasia wrote:
> Seems short enough to introduce an extra variable here. :)
removed the variable
Comment at: lib/CodeGen/CGAtomic.cpp:707
- switch (E->getOp()) {
+ auto Op = E->getOp();
+ switch (Op) {
Anastasia wrote:
> The same here... not sure adding an extra variable is helping here. :)
removed the variable
Comment at: lib/CodeGen/CGAtomic.cpp:889
+return V;
+ auto OrigLangAS = E->getType()
+.getTypePtr()
Anastasia wrote:
> Formatting seems to be a bit odd here...
this is done by clang-format
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+ "Non-constant synchronization scope not supported");
+ auto sco = (llvm::SynchronizationScope)(
+ cast(Scope)->getZExtValue());
Anastasia wrote:
> Anastasia wrote:
> > Variable name doesn't follow the style.
> could we avoid using C style cast?
will fix
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+ "Non-constant synchronization scope not supported");
+ auto sco = (llvm::SynchronizationScope)(
+ cast(Scope)->getZExtValue());
yaxunl wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > Variable name doesn't follow the style.
> > could we avoid using C style cast?
> will fix
will change to static_cast
Comment at: lib/Sema/SemaChecking.cpp:3146
+Op == AtomicExpr::AO__opencl_atomic_load)
+? 0
+: 1);
Anastasia wrote:
> formatting seems odd.
this is done by clang-format
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:1
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64
-emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple
armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
Anastasia wrote:
> GEN4 -> SPIR
will change
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:2
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64
-emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple
armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
+
Anastasia wrote:
> GEN0 -> AMDGPU
Actually this triple is armv5e. This test requires a target not supporting
atomic instructions. Will change GEN0 -> ARM
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:4
+
+void f(atomic_int *i, int cmp) {
+ int x;
Anastasia wrote:
> Could we use different scopes?
Yes. will add them.
Comment at: test/CodeGenOpenCL/atomic-ops.cl:7
+
+#ifndef ALREADY_INCLUDED
+#define ALREADY_INCLUDED
Anastasia wrote:
> why do we need this?
This is to test the builtin works in pch. When generating pch, ALREADY_INCLUDED
is undefined, therefore pch will include all functions. When including pch,
since ALREADY_INCLUDED is defined through pch, the cl file is empty and
function definitions from pch is used for codegen.
Comment at: test/CodeGenOpenCL/atomic-ops.cl:15
+ // CHECK: load atomic i32, i32 addrspace(4)* %{{[.0-9A-Z_a-z]+}}
singlethread seq_cst
+ int x = __opencl_atomic_load(i, memory_order_seq_cst,
memory_scope_work_item);
+}
Anastasia wrote:
> I think we could use different scope types all over...
will add them.
Comment at: test/CodeGenOpenCL/atomic-ops.cl:32
+ // CHECK-LABEL: @fi4(
+ // CHECK: [[PAIR:%[.0-9A-Z_a-z]+]] = cmpxchg i32 addrspace(4)*
[[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32
[[DESIRED:%[.0-9A-Z_a-z]+]] singlethread acquire
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
Anastasia added inline comments.
Comment at: include/clang/Basic/Builtins.def:713
+ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t")
+ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t")
+
What about min/max? I believe they will need to have the scope too.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6956
+ "synchronization scope argument to atomic operation is invalid">;
+def err_atomic_op_has_non_constant_synch_scope : Error<
+ "non-constant synchronization scope argument to atomic operation is not
supported">;
Btw, is this disallowed by the spec? Can't find anything relevant.
Comment at: lib/CodeGen/CGAtomic.cpp:678
RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
+ bool IsOpenCL = E->isOpenCL();
QualType AtomicTy = E->getPtr()->getType()->getPointeeType();
Seems short enough to introduce an extra variable here. :)
Comment at: lib/CodeGen/CGAtomic.cpp:707
- switch (E->getOp()) {
+ auto Op = E->getOp();
+ switch (Op) {
The same here... not sure adding an extra variable is helping here. :)
Comment at: lib/CodeGen/CGAtomic.cpp:889
+return V;
+ auto OrigLangAS = E->getType()
+.getTypePtr()
Formatting seems to be a bit odd here...
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+ "Non-constant synchronization scope not supported");
+ auto sco = (llvm::SynchronizationScope)(
+ cast(Scope)->getZExtValue());
Variable name doesn't follow the style.
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+ "Non-constant synchronization scope not supported");
+ auto sco = (llvm::SynchronizationScope)(
+ cast(Scope)->getZExtValue());
Anastasia wrote:
> Variable name doesn't follow the style.
could we avoid using C style cast?
Comment at: lib/Sema/SemaChecking.cpp:3146
+Op == AtomicExpr::AO__opencl_atomic_load)
+? 0
+: 1);
formatting seems odd.
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:1
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64
-emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple
armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
GEN4 -> SPIR
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:2
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64
-emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple
armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
+
GEN0 -> AMDGPU
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:4
+
+void f(atomic_int *i, int cmp) {
+ int x;
Could we use different scopes?
Comment at: test/CodeGenOpenCL/atomic-ops.cl:7
+
+#ifndef ALREADY_INCLUDED
+#define ALREADY_INCLUDED
why do we need this?
Comment at: test/CodeGenOpenCL/atomic-ops.cl:15
+ // CHECK: load atomic i32, i32 addrspace(4)* %{{[.0-9A-Z_a-z]+}}
singlethread seq_cst
+ int x = __opencl_atomic_load(i, memory_order_seq_cst,
memory_scope_work_item);
+}
I think we could use different scope types all over...
Comment at: test/CodeGenOpenCL/atomic-ops.cl:32
+ // CHECK-LABEL: @fi4(
+ // CHECK: [[PAIR:%[.0-9A-Z_a-z]+]] = cmpxchg i32 addrspace(4)*
[[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32
[[DESIRED:%[.0-9A-Z_a-z]+]] singlethread acquire acquire
+ // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = extractvalue { i32, i1 } [[PAIR]], 0
do we have an addrspacecast before?
Comment at: test/SemaOpenCL/atomic-ops.cl:22
+ intptr_t *P, float *D, struct S *s1, struct S *s2) {
+ __opencl_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic
operation must be a pointer to non-const _Atomic type ('const __generic
atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
could we have the full error for consistency?
Comment at: test/SemaOpenCL/atomic-ops.cl:30
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item);
// expected-error {{address argument to atomic operation must be a pointer to
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic
_Atomic(int) *') invalid)}}
+
could we also test with constant AS? Also I would generally improve testing wrt
address spaces...
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 102374.
yaxunl marked 11 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExpr.cpp
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,146 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2) {
+ __opencl_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function}}
+ __opencl_atomic_load(0,0,0,0); // expected-error {{too many arguments to function}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int*)0,0,0,0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{operand of type 'void'}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{must be a pointer to atomic integer or pointer}}
+
+ __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{must be a pointer to atomic integer}}
+
+ bool cmpexch_1 = __opencl_atomic_compare_exchange_strong(i, I, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item);
+ bool cmpexch_2 = __opencl_atomic_compare_exchange_strong(p, P, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item);
+ bool cmpexch_3 = __opencl_atomic_compare_exchange_strong(d, I, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item); // expected-warning {{incompatible pointer types}}
+ (void)__opencl_atomic_compare_exchange_strong(i, CI, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item); // expected-warning {{passing 'const __generic int *' to parameter of type '__generic int *' discards qualifiers}}
+
+ bool cmpexchw_1 = __opencl_atomic_compare_exchange_weak(i, I, 1,
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
rjmccall added a comment.
Looks like you haven't introduced proper constants in the header for scopes.
Comment at: docs/LanguageExtensions.rst:1855
+atomic builtins are in ``explicit`` form of the corresponding OpenCL 2.0
+builtin function, and are named with a ``__opencl__`` prefix.)
1. "an", not "in".
2. There's no need to put "explicit" in `` quotes.
3. The prefix you're using is "__opencl_", with only one trailing underscore.
Comment at: lib/CodeGen/CGAtomic.cpp:736
OrderFail = EmitScalarExpr(E->getOrderFail());
-if (E->getNumSubExprs() == 6)
+if (E->getNumSubExprs() == 7)
IsWeak = EmitScalarExpr(E->getWeak());
This is equivalent to checking for the two AO kinds, right? Probably better to
just do that.
Comment at: lib/CodeGen/CGExpr.cpp:50
+llvm::Value *CodeGenFunction::EmitCastToVoidPtr(llvm::Value *value,
+bool CastToGenericAddrSpace) {
+ unsigned addressSpace;
I think it would be better to keep this function simple and just add the cast
outside in the two places you need it.
Also, please remember to use the target hook instead of using an addrspacecast
directly.
Comment at: lib/Sema/SemaChecking.cpp:3046
// The order(s) are always converted to int.
Ty = Context.IntTy;
}
This clause now covers the scope argument as well.
Comment at: lib/Sema/SemaChecking.cpp:3060
+ if (IsOpenCL) {
+Scope = TheCall->getArg(TheCall->getNumArgs() - 1);
+ } else {
You need to check that is a constant expression in the correct range, and you
should add a test for that.
Comment at: lib/Sema/SemaChecking.cpp:3062
+ } else {
+Scope = IntegerLiteral::Create(
+Context, llvm::APInt(Context.getTypeSize(Context.IntTy), (uint64_t)1),
Please document the meaning of 1.
https://reviews.llvm.org/D28691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
yaxunl updated this revision to Diff 101955.
yaxunl retitled this revision from "Support synchronisation scope in Clang
atomic builtin functions" to "Add OpenCL 2.0 atomic builtin functions as Clang
builtin".
yaxunl edited the summary of this revision.
yaxunl added a comment.
Add __opencl_atomic_ builtins.
https://reviews.llvm.org/D28691
Files:
docs/LanguageExtensions.rst
include/clang/AST/Expr.h
include/clang/Basic/Builtins.def
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/StmtPrinter.cpp
lib/Basic/Targets.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CodeGenFunction.h
lib/Headers/opencl-c.h
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/SemaOpenCL/atomic-ops.cl
Index: test/SemaOpenCL/atomic-ops.cl
===
--- /dev/null
+++ test/SemaOpenCL/atomic-ops.cl
@@ -0,0 +1,141 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+
+// Basic parsing/Sema tests for __opencl_atomic_*
+
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
+struct S { char c[3]; };
+
+char i8;
+short i16;
+int i32;
+int8 i64;
+
+atomic_int gn;
+
+void f(atomic_int *i, const atomic_int *ci,
+ atomic_intptr_t *p, atomic_float *d,
+ int *I, const int *CI,
+ intptr_t *P, float *D, struct S *s1, struct S *s2) {
+ __opencl_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(0); // expected-error {{too few arguments to function}}
+ __opencl_atomic_load(0,0,0,0); // expected-error {{too many arguments to function}}
+ __opencl_atomic_store(0,0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
+ __opencl_atomic_store((int*)0,0,0,0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
+ __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{operand of type 'void'}}
+
+ int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{must be a pointer to _Atomic}}
+
+ __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{must be a pointer to atomic integer or pointer}}
+
+ __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_item);
+ __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_item); // expected-error {{must be a pointer to atomic integer}}
+
+ bool cmpexch_1 = __opencl_atomic_compare_exchange_strong(i, I, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item);
+ bool cmpexch_2 = __opencl_atomic_compare_exchange_strong(p, P, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item);
+ bool cmpexch_3 = __opencl_atomic_compare_exchange_strong(d, I, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item); // expected-warning {{incompatible pointer types}}
+ (void)__opencl_atomic_compare_exchange_strong(i, CI, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_item); // expected-warning {{passing 'const __generic int *' to parameter of
