[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-04 Thread Yaxun Liu via Phabricator via cfe-commits
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&id=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 &CGM, 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 &C) 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 &C) const {
+  return C.getOrInsertSyncScopeID(""); /* default sync scope */
+}
+
 static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays);
 
 /// isEmptyField - Return true iff a the field is "empty", that is it
@@ -7430,6 +7435,8 @@
   }
   unsigned getGlobalVarAddressSpace(CodeGenModule &CGM,
 const VarDecl *D) const override;
+  llvm::SyncScope::ID getLLVMSyncScopeID(SyncScope S,
+ llvm::LLVMContext &C) const override;
 };
 }
 
@@ -7539,6 +7546,26 @@
   return DefaultGlobalAS;
 }
 
+llvm::SyncScope::ID
+AMDGPUTargetCodeGenInfo::getLLVMSyncScopeID(SyncScope S,
+llvm::LLVMContext &C) 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 w

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-04 Thread John McCall via Phabricator via cfe-commits
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

2017-08-04 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-08-04 Thread Alexey Bader via Phabricator via cfe-commits
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

2017-08-04 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-08-03 Thread Tony Tye via Phabricator via cfe-commits
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

2017-08-03 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-08-03 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-08-03 Thread Tony Tye via Phabricator via cfe-commits
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

2017-08-03 Thread Yaxun Liu via Phabricator via cfe-commits
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)}}
+  __opencl

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-03 Thread John McCall via Phabricator via cfe-commits
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

2017-08-03 Thread Yaxun Liu via Phabricator via cfe-commits
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 ('__gene

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-03 Thread Yaxun Liu via Phabricator via cfe-commits
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 int

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-03 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-08-02 Thread John McCall via Phabricator via cfe-commits
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

2017-08-02 Thread Tony Tye via Phabricator via cfe-commits
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

2017-08-02 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-07-28 Thread John McCall via Phabricator via cfe-commits
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

2017-07-28 Thread John McCall via Phabricator via cfe-commits
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

2017-07-28 Thread Anastasia Stulova via Phabricator via cfe-commits
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

2017-07-25 Thread Tony Tye via Phabricator via cfe-commits
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

2017-07-25 Thread John McCall via Phabricator via cfe-commits
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

2017-07-25 Thread Brian Sumner via Phabricator via cfe-commits
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

2017-07-25 Thread John McCall via Phabricator via cfe-commits
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

2017-07-25 Thread Brian Sumner via Phabricator via cfe-commits
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

2017-07-25 Thread John McCall via Phabricator via cfe-commits
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

2017-07-25 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-07-25 Thread Yaxun Liu via Phabricator via cfe-commits
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);
+  __opencl_atomic_fetc

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-07-25 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-07-25 Thread Brian Sumner via Phabricator via cfe-commits
b-sumner added a comment.

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.


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

2017-07-25 Thread Konstantin Zhuravlyov via Phabricator via cfe-commits
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

2017-07-25 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-07-25 Thread Brian Sumner via Phabricator via cfe-commits
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

2017-07-25 Thread Yaxun Liu via Phabricator via cfe-commits
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

2017-07-25 Thread Yaxun Liu via Phabricator via cfe-commits
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 acqu

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-06-20 Thread Anastasia Stulova via Phabricator via cfe-commits
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...


https://reviews.llvm.o

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-06-13 Thread Yaxun Liu via Phabricator via cfe-commits
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, mem

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-06-09 Thread John McCall via Phabricator via cfe-commits
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

2017-06-08 Thread Yaxun Liu via Phabricator via cfe-commits
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 type