[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: cfe/trunk/lib/Frontend/InitPreprocessor.cpp:581
   // Define macros for the OpenCL memory scope.
   // The values should match clang SyncScope enum.
+  static_assert(

t-tye wrote:
> // The values should match clang AtomicScopeOpenCLModel::ID enum.
Fixed. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: cfe/trunk/lib/Frontend/InitPreprocessor.cpp:581
   // Define macros for the OpenCL memory scope.
   // The values should match clang SyncScope enum.
+  static_assert(

// The values should match clang AtomicScopeOpenCLModel::ID enum.


Repository:
  rL LLVM

https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310924: [OpenCL] Support variable memory scope in atomic 
builtins (authored by yaxunl).

Changed prior to commit:
  https://reviews.llvm.org/D36580?vs=60=85#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36580

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Basic/SyncScope.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/CodeGen/CGAtomic.cpp
  cfe/trunk/lib/Frontend/InitPreprocessor.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/CodeGenOpenCL/atomic-ops-libcall.cl
  cfe/trunk/test/CodeGenOpenCL/atomic-ops.cl
  cfe/trunk/test/SemaOpenCL/atomic-ops.cl

Index: cfe/trunk/lib/CodeGen/CGAtomic.cpp
===
--- cfe/trunk/lib/CodeGen/CGAtomic.cpp
+++ cfe/trunk/lib/CodeGen/CGAtomic.cpp
@@ -18,6 +18,7 @@
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Operator.h"
@@ -659,6 +660,61 @@
   return DeclPtr;
 }
 
+static void EmitAtomicOp(CodeGenFunction , AtomicExpr *Expr, Address Dest,
+ Address Ptr, Address Val1, Address Val2,
+ llvm::Value *IsWeak, llvm::Value *FailureOrder,
+ uint64_t Size, llvm::AtomicOrdering Order,
+ llvm::Value *Scope) {
+  auto ScopeModel = Expr->getScopeModel();
+
+  // LLVM atomic instructions always have synch scope. If clang atomic
+  // expression has no scope operand, use default LLVM synch scope.
+  if (!ScopeModel) {
+EmitAtomicOp(CGF, Expr, Dest, Ptr, Val1, Val2, IsWeak, FailureOrder, Size,
+ Order, CGF.CGM.getLLVMContext().getOrInsertSyncScopeID(""));
+return;
+  }
+
+  // Handle constant scope.
+  if (auto SC = dyn_cast(Scope)) {
+auto SCID = CGF.getTargetHooks().getLLVMSyncScopeID(
+ScopeModel->map(SC->getZExtValue()), CGF.CGM.getLLVMContext());
+EmitAtomicOp(CGF, Expr, Dest, Ptr, Val1, Val2, IsWeak, FailureOrder, Size,
+ Order, SCID);
+return;
+  }
+
+  // Handle non-constant scope.
+  auto  = CGF.Builder;
+  auto Scopes = ScopeModel->getRuntimeValues();
+  llvm::DenseMap BB;
+  for (auto S : Scopes)
+BB[S] = CGF.createBasicBlock(getAsString(ScopeModel->map(S)), CGF.CurFn);
+
+  llvm::BasicBlock *ContBB =
+  CGF.createBasicBlock("atomic.scope.continue", CGF.CurFn);
+
+  auto *SC = Builder.CreateIntCast(Scope, Builder.getInt32Ty(), false);
+  // If unsupported synch scope is encountered at run time, assume a fallback
+  // synch scope value.
+  auto FallBack = ScopeModel->getFallBackValue();
+  llvm::SwitchInst *SI = Builder.CreateSwitch(SC, BB[FallBack]);
+  for (auto S : Scopes) {
+auto *B = BB[S];
+if (S != FallBack)
+  SI->addCase(Builder.getInt32(S), B);
+
+Builder.SetInsertPoint(B);
+EmitAtomicOp(CGF, Expr, Dest, Ptr, Val1, Val2, IsWeak, FailureOrder, Size,
+ Order,
+ CGF.getTargetHooks().getLLVMSyncScopeID(ScopeModel->map(S),
+ CGF.getLLVMContext()));
+Builder.CreateBr(ContBB);
+  }
+
+  Builder.SetInsertPoint(ContBB);
+}
+
 static void
 AddDirectArgument(CodeGenFunction , CallArgList ,
   bool UseOptimizedLibcall, llvm::Value *Val, QualType ValTy,
@@ -711,7 +767,8 @@
   }
 
   llvm::Value *Order = EmitScalarExpr(E->getOrder());
-  llvm::Value *Scope = EmitScalarExpr(E->getScope());
+  llvm::Value *Scope =
+  E->getScopeModel() ? EmitScalarExpr(E->getScope()) : nullptr;
 
   switch (E->getOp()) {
   case AtomicExpr::AO__c11_atomic_init:
@@ -1132,44 +1189,38 @@
 E->getOp() == AtomicExpr::AO__atomic_load ||
 E->getOp() == AtomicExpr::AO__atomic_load_n;
 
-  assert(isa(Scope) &&
-  "Non-constant synchronization scope not supported");
-  auto SCID = getTargetHooks().getLLVMSyncScopeID(
-  static_cast(cast(Scope)->getZExtValue()),
-  getLLVMContext());
-
   if (isa(Order)) {
 auto ord = cast(Order)->getZExtValue();
 // We should not ever get to a case where the ordering isn't a valid C ABI
 // value, but it's hard to enforce that in general.
 if (llvm::isValidAtomicOrderingCABI(ord))
   switch ((llvm::AtomicOrderingCABI)ord) {
   case llvm::AtomicOrderingCABI::relaxed:
 EmitAtomicOp(*this, E, Dest, Ptr, Val1, Val2, IsWeak, OrderFail, Size,
- llvm::AtomicOrdering::Monotonic, SCID);
+ llvm::AtomicOrdering::Monotonic, Scope);
 break;
   case llvm::AtomicOrderingCABI::consume:
   case llvm::AtomicOrderingCABI::acquire:
 if (IsStore)
   break; // Avoid crashing on code with undefined behavior
 

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Thanks, that looks great.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 60.
yaxunl marked 3 inline comments as done.
yaxunl added a comment.

Revised by John's comments.


https://reviews.llvm.org/D36580

Files:
  include/clang/AST/Expr.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/SyncScope.h
  lib/AST/Expr.cpp
  lib/CodeGen/CGAtomic.cpp
  lib/Frontend/InitPreprocessor.cpp
  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
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -14,7 +14,6 @@
 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,
@@ -81,6 +80,13 @@
 }
 
 void memory_checks(atomic_int *Ap, int *p, int val) {
+  // non-integer memory order argument is casted to integer type.
+  (void)__opencl_atomic_load(Ap, 1.0f, memory_scope_work_group);
+  float forder;
+  (void)__opencl_atomic_load(Ap, forder, memory_scope_work_group);
+  struct S s;
+  (void)__opencl_atomic_load(Ap, s, memory_scope_work_group); // expected-error {{passing 'struct S' to parameter of incompatible type 'int'}}
+
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_acquire, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_consume, memory_scope_work_group); // expected-error {{use of undeclared identifier 'memory_order_consume'}}
@@ -151,8 +157,15 @@
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_device);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_all_svm_devices);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_sub_group);
-  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope); // expected-error{{non-constant synchronization scope argument to atomic operation is not supported}}
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, 10);//expected-error{{synchronization scope argument to atomic operation is invalid}}
+
+  // non-integer memory scope is casted to integer type.
+  float fscope;
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, 1.0f);
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, fscope);
+  struct S s;
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, s); //expected-error{{passing 'struct S' to parameter of incompatible type 'int'}}
 }
 
 void nullPointerWarning(atomic_int *Ap, int *p, int val) {
Index: test/CodeGenOpenCL/atomic-ops.cl
===
--- test/CodeGenOpenCL/atomic-ops.cl
+++ test/CodeGenOpenCL/atomic-ops.cl
@@ -52,6 +52,81 @@
   return __opencl_atomic_compare_exchange_strong(i, , 1, memory_order_acquire, memory_order_acquire, memory_scope_work_group);
 }
 
+void fi5(atomic_int *i, int scope) {
+  // CHECK-LABEL: @fi5
+  // CHECK: switch i32 %{{.*}}, label %opencl_allsvmdevices [
+  // CHECK-NEXT: i32 1, label %opencl_workgroup
+  // CHECK-NEXT: i32 2, label %opencl_device
+  // CHECK-NEXT: i32 4, label %opencl_subgroup
+  // CHECK-NEXT: ]
+  // CHECK: opencl_workgroup:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_device:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_allsvmdevices:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_subgroup:
+  // CHECK: %5 = load atomic i32, i32 addrspace(4)* %0 syncscope("subgroup") seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: atomic.scope.continue:
+  int x = __opencl_atomic_load(i, memory_order_seq_cst, scope);
+}
+
+void fi6(atomic_int *i, int order, int scope) {
+  // CHECK-LABEL: @fi6
+  // CHECK: switch i32 %{{.*}}, label %monotonic [
+  // CHECK-NEXT: i32 1, label %acquire
+  // CHECK-NEXT: i32 2, label %acquire
+  // CHECK-NEXT: i32 5, label %seqcst
+  // CHECK-NEXT: ]
+  // CHECK: monotonic:
+  // CHECK: switch i32 %{{.*}}, label %[[MON_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[MON_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[MON_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[MON_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: acquire:
+  // CHECK: switch i32 %{{.*}}, label %[[ACQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[ACQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[ACQ_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[ACQ_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: seqcst:
+  // CHECK: switch i32 %2, label %[[SEQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[SEQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[SEQ_DEV:.*]]
+  // CHECK-NEXT: i32 

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 6 inline comments as done.
yaxunl added inline comments.



Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > Please correct me if I'm wrong, but I believe you don't get to define 
> > > > > the "ABI for OpenCL" any more than I get to define the "ABI for C".  
> > > > > You're defining the ABI for your specific OpenCL implementation, but 
> > > > > Clang might reasonably support other implementations with their own 
> > > > > ABIs.  So this name is inappropriate.
> > > > > 
> > > > > Now, if I understand how SPIR works, SPIR does require some sort of 
> > > > > stable lowering of these atomic operations in order to ensure that 
> > > > > the resulting LLVM IR can be ingested into an arbitrary 
> > > > > implementation.  (Ot at least that was true prior to SPIR V?)  
> > > > > Therefore, SPIR needs to specify a lowering for these atomic 
> > > > > operations, and that probably includes ABI values for specific 
> > > > > sync-scopes.  But the appropriate name for that would be the "SPIR 
> > > > > ABI", not the "OpenCL ABI".  And, of course, you would need to be 
> > > > > sure that you're implementing the lowering that SPIR actually wants.
> > > > The ABI is not intended for a specific OpenCL implementation, but for 
> > > > extending to other languages. For example, we have a downstream C++ 
> > > > based language called HCC, which may support atomic scope with an ABI 
> > > > different than OpenCL atomic scope ABI.
> > > By "ABI" you mean that it might present a different model of 
> > > synchronization scopes to the source language?  Okay, that's great, and 
> > > it explains a lot of things that were very murky about some of our 
> > > previous conversations.
> > > 
> > > In that case, I think the appropriate design for these builtins is:
> > > 
> > >   - They don't need to be in the __builtin_opencl namespace.
> > >   - They *do* need to be enabled only in language modes with well-defined 
> > > sync-scope models, which for now just means OpenCL. 
> > >   - The language mode's sync-scope model should determine everything 
> > > about the scope argument, including its type.  Sema-level validation 
> > > requires first determining the language's sync-scope model.
> > >   - There is no meaningful way to "default" the scope argument on the 
> > > non-scoped builtins the way that we are now.  Instead, the scope argument 
> > > should only exist for the scoped builtins.
> > > 
> > > Alternatively, if you potentially want the opencl-model builtins to be 
> > > usable from non-opencl languages, the right design is for HCC to use its 
> > > own set of builtins with their own type-checking rules.
> > > 
> > > In either case, I don't think it's a good idea to call this "ABI", which 
> > > is associated too strongly with target-lowering concepts.  You're really 
> > > talking about a source-language concept.
> > We could introduce __builtin_hcc_atomic_* in downstream HCC.
> > 
> > We need a generic approach to represent atomic scope values for different 
> > languages and associated semantic/codegen needs, and AtomicScopeABI seems 
> > generic enough to serve the purpose. It has interface for semantic checking 
> > (isValid), and the interface to map to LLVM sync scope for both constant 
> > and variable arguments. If the name is not proper, we could rename it to 
> > just `AtomicScope`.
> > 
> > I can remove the default atomic scope for atomic expressions without scope 
> > originally. For them I just emit LLVM atomic instructions with default LLVM 
> > synch scope.
> I agree that there's value in having a generic approach even if you're going 
> to have different builtins for different scope models.  However, if you do 
> use different builtins for different scope models, you need to make sure that 
> you pick the appropriate model for the builtin being used, not according to 
> the current language mode.  That is, there should be some way to map a 
> builtin ID to an AtomicScopeModelKind (values: None, OpenCL, (future) HCC), 
> and then you can use that to create an AtomicScopeModel, which can have all 
> these methods on it.
> 
> I would encourage you to go ahead and give AtomicScopeModel an implementation 
> file and move more of these method definitions into that.
Thanks. Will do.



Comment at: include/clang/Basic/SyncScope.h:105
+
+  AtomicScopeOpenCLABI() {}
+

rjmccall wrote:
> I would call this OpenCLAtomicScopeModel.
Will do.



Comment at: include/clang/Basic/SyncScope.h:128
+static_assert(Last == SubGroup, "Does not include all synch scopes");
+static unsigned Scopes[] = {
+static_cast(WorkGroup), static_cast(Device),

rjmccall wrote:
> Make this const, please.

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > Please correct me if I'm wrong, but I believe you don't get to define 
> > > > the "ABI for OpenCL" any more than I get to define the "ABI for C".  
> > > > You're defining the ABI for your specific OpenCL implementation, but 
> > > > Clang might reasonably support other implementations with their own 
> > > > ABIs.  So this name is inappropriate.
> > > > 
> > > > Now, if I understand how SPIR works, SPIR does require some sort of 
> > > > stable lowering of these atomic operations in order to ensure that the 
> > > > resulting LLVM IR can be ingested into an arbitrary implementation.  
> > > > (Ot at least that was true prior to SPIR V?)  Therefore, SPIR needs to 
> > > > specify a lowering for these atomic operations, and that probably 
> > > > includes ABI values for specific sync-scopes.  But the appropriate name 
> > > > for that would be the "SPIR ABI", not the "OpenCL ABI".  And, of 
> > > > course, you would need to be sure that you're implementing the lowering 
> > > > that SPIR actually wants.
> > > The ABI is not intended for a specific OpenCL implementation, but for 
> > > extending to other languages. For example, we have a downstream C++ based 
> > > language called HCC, which may support atomic scope with an ABI different 
> > > than OpenCL atomic scope ABI.
> > By "ABI" you mean that it might present a different model of 
> > synchronization scopes to the source language?  Okay, that's great, and it 
> > explains a lot of things that were very murky about some of our previous 
> > conversations.
> > 
> > In that case, I think the appropriate design for these builtins is:
> > 
> >   - They don't need to be in the __builtin_opencl namespace.
> >   - They *do* need to be enabled only in language modes with well-defined 
> > sync-scope models, which for now just means OpenCL. 
> >   - The language mode's sync-scope model should determine everything about 
> > the scope argument, including its type.  Sema-level validation requires 
> > first determining the language's sync-scope model.
> >   - There is no meaningful way to "default" the scope argument on the 
> > non-scoped builtins the way that we are now.  Instead, the scope argument 
> > should only exist for the scoped builtins.
> > 
> > Alternatively, if you potentially want the opencl-model builtins to be 
> > usable from non-opencl languages, the right design is for HCC to use its 
> > own set of builtins with their own type-checking rules.
> > 
> > In either case, I don't think it's a good idea to call this "ABI", which is 
> > associated too strongly with target-lowering concepts.  You're really 
> > talking about a source-language concept.
> We could introduce __builtin_hcc_atomic_* in downstream HCC.
> 
> We need a generic approach to represent atomic scope values for different 
> languages and associated semantic/codegen needs, and AtomicScopeABI seems 
> generic enough to serve the purpose. It has interface for semantic checking 
> (isValid), and the interface to map to LLVM sync scope for both constant and 
> variable arguments. If the name is not proper, we could rename it to just 
> `AtomicScope`.
> 
> I can remove the default atomic scope for atomic expressions without scope 
> originally. For them I just emit LLVM atomic instructions with default LLVM 
> synch scope.
I agree that there's value in having a generic approach even if you're going to 
have different builtins for different scope models.  However, if you do use 
different builtins for different scope models, you need to make sure that you 
pick the appropriate model for the builtin being used, not according to the 
current language mode.  That is, there should be some way to map a builtin ID 
to an AtomicScopeModelKind (values: None, OpenCL, (future) HCC), and then you 
can use that to create an AtomicScopeModel, which can have all these methods on 
it.

I would encourage you to go ahead and give AtomicScopeModel an implementation 
file and move more of these method definitions into that.



Comment at: include/clang/Basic/SyncScope.h:105
+
+  AtomicScopeOpenCLABI() {}
+

I would call this OpenCLAtomicScopeModel.



Comment at: include/clang/Basic/SyncScope.h:128
+static_assert(Last == SubGroup, "Does not include all synch scopes");
+static unsigned Scopes[] = {
+static_cast(WorkGroup), static_cast(Device),

Make this const, please.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added inline comments.



Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Please correct me if I'm wrong, but I believe you don't get to define the 
> > > "ABI for OpenCL" any more than I get to define the "ABI for C".  You're 
> > > defining the ABI for your specific OpenCL implementation, but Clang might 
> > > reasonably support other implementations with their own ABIs.  So this 
> > > name is inappropriate.
> > > 
> > > Now, if I understand how SPIR works, SPIR does require some sort of 
> > > stable lowering of these atomic operations in order to ensure that the 
> > > resulting LLVM IR can be ingested into an arbitrary implementation.  (Ot 
> > > at least that was true prior to SPIR V?)  Therefore, SPIR needs to 
> > > specify a lowering for these atomic operations, and that probably 
> > > includes ABI values for specific sync-scopes.  But the appropriate name 
> > > for that would be the "SPIR ABI", not the "OpenCL ABI".  And, of course, 
> > > you would need to be sure that you're implementing the lowering that SPIR 
> > > actually wants.
> > The ABI is not intended for a specific OpenCL implementation, but for 
> > extending to other languages. For example, we have a downstream C++ based 
> > language called HCC, which may support atomic scope with an ABI different 
> > than OpenCL atomic scope ABI.
> By "ABI" you mean that it might present a different model of synchronization 
> scopes to the source language?  Okay, that's great, and it explains a lot of 
> things that were very murky about some of our previous conversations.
> 
> In that case, I think the appropriate design for these builtins is:
> 
>   - They don't need to be in the __builtin_opencl namespace.
>   - They *do* need to be enabled only in language modes with well-defined 
> sync-scope models, which for now just means OpenCL. 
>   - The language mode's sync-scope model should determine everything about 
> the scope argument, including its type.  Sema-level validation requires first 
> determining the language's sync-scope model.
>   - There is no meaningful way to "default" the scope argument on the 
> non-scoped builtins the way that we are now.  Instead, the scope argument 
> should only exist for the scoped builtins.
> 
> Alternatively, if you potentially want the opencl-model builtins to be usable 
> from non-opencl languages, the right design is for HCC to use its own set of 
> builtins with their own type-checking rules.
> 
> In either case, I don't think it's a good idea to call this "ABI", which is 
> associated too strongly with target-lowering concepts.  You're really talking 
> about a source-language concept.
We could introduce __builtin_hcc_atomic_* in downstream HCC.

We need a generic approach to represent atomic scope values for different 
languages and associated semantic/codegen needs, and AtomicScopeABI seems 
generic enough to serve the purpose. It has interface for semantic checking 
(isValid), and the interface to map to LLVM sync scope for both constant and 
variable arguments. If the name is not proper, we could rename it to just 
`AtomicScope`.

I can remove the default atomic scope for atomic expressions without scope 
originally. For them I just emit LLVM atomic instructions with default LLVM 
synch scope.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:

yaxunl wrote:
> rjmccall wrote:
> > Please correct me if I'm wrong, but I believe you don't get to define the 
> > "ABI for OpenCL" any more than I get to define the "ABI for C".  You're 
> > defining the ABI for your specific OpenCL implementation, but Clang might 
> > reasonably support other implementations with their own ABIs.  So this name 
> > is inappropriate.
> > 
> > Now, if I understand how SPIR works, SPIR does require some sort of stable 
> > lowering of these atomic operations in order to ensure that the resulting 
> > LLVM IR can be ingested into an arbitrary implementation.  (Ot at least 
> > that was true prior to SPIR V?)  Therefore, SPIR needs to specify a 
> > lowering for these atomic operations, and that probably includes ABI values 
> > for specific sync-scopes.  But the appropriate name for that would be the 
> > "SPIR ABI", not the "OpenCL ABI".  And, of course, you would need to be 
> > sure that you're implementing the lowering that SPIR actually wants.
> The ABI is not intended for a specific OpenCL implementation, but for 
> extending to other languages. For example, we have a downstream C++ based 
> language called HCC, which may support atomic scope with an ABI different 
> than OpenCL atomic scope ABI.
By "ABI" you mean that it might present a different model of synchronization 
scopes to the source language?  Okay, that's great, and it explains a lot of 
things that were very murky about some of our previous conversations.

In that case, I think the appropriate design for these builtins is:

  - They don't need to be in the __builtin_opencl namespace.
  - They *do* need to be enabled only in language modes with well-defined 
sync-scope models, which for now just means OpenCL. 
  - The language mode's sync-scope model should determine everything about the 
scope argument, including its type.  Sema-level validation requires first 
determining the language's sync-scope model.
  - There is no meaningful way to "default" the scope argument on the 
non-scoped builtins the way that we are now.  Instead, the scope argument 
should only exist for the scoped builtins.

Alternatively, if you potentially want the opencl-model builtins to be usable 
from non-opencl languages, the right design is for HCC to use its own set of 
builtins with their own type-checking rules.

In either case, I don't think it's a good idea to call this "ABI", which is 
associated too strongly with target-lowering concepts.  You're really talking 
about a source-language concept.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 7 inline comments as done.
yaxunl added inline comments.



Comment at: include/clang/Basic/SyncScope.h:89
+  static std::unique_ptr create(LangOptions );
+};
+

rjmccall wrote:
> The previous design of this header seems more appropriate to me.  Clang 
> defines a builtin with standardized argument values for all targets; we're 
> not trying to allow targets to customize the arguments taken by this builtin, 
> at least not yet.  The sync-scope argument values are the values from this 
> enum. Because they are potentially exposed in source programs, they cannot be 
> lightly changed and re-ordered; that is, they are ABI.  There is a second 
> level of ABI, which is the set of values demanded by target runtime 
> functions.  It is IRGen's responsibility to turn the SyncScope values into 
> those values when generating a call to the runtime; that mapping does not 
> need to be exposed here in the AST.
This function is not intended to create ABI's for different targets, but to 
create ABI's for different languages. Currently only OpenCL supports atomic 
scope, but in the future there may be other languages which support atomic 
scope with a different ABI.



Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:

rjmccall wrote:
> Please correct me if I'm wrong, but I believe you don't get to define the 
> "ABI for OpenCL" any more than I get to define the "ABI for C".  You're 
> defining the ABI for your specific OpenCL implementation, but Clang might 
> reasonably support other implementations with their own ABIs.  So this name 
> is inappropriate.
> 
> Now, if I understand how SPIR works, SPIR does require some sort of stable 
> lowering of these atomic operations in order to ensure that the resulting 
> LLVM IR can be ingested into an arbitrary implementation.  (Ot at least that 
> was true prior to SPIR V?)  Therefore, SPIR needs to specify a lowering for 
> these atomic operations, and that probably includes ABI values for specific 
> sync-scopes.  But the appropriate name for that would be the "SPIR ABI", not 
> the "OpenCL ABI".  And, of course, you would need to be sure that you're 
> implementing the lowering that SPIR actually wants.
The ABI is not intended for a specific OpenCL implementation, but for extending 
to other languages. For example, we have a downstream C++ based language called 
HCC, which may support atomic scope with an ABI different than OpenCL atomic 
scope ABI.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Basic/SyncScope.h:59
+return "opencl_subgroup";
+  }
+}

yaxunl wrote:
> rjmccall wrote:
> > t-tye wrote:
> > > Should there be a default/assert/static_assert to allow SyncScope enum to 
> > > grow due to other languages?
> > There will already be a warning from -Wswitch about this, which should be 
> > sufficient.
> > 
> > But we do often add llvm_unreachable after switches like this.
> I tried adding
> 
> ```
> default:
> llvm_unreachable("Invalid sync scope");
> ```
> 
> However, it results in error:
> 
> ```
>  error: default label in switch which covers all enumeration values 
> [-Werror,-Wcovered-switch-default]
> ```
> I guess I'd better leave it as it is since we already have -Wswitch.
Yeah, the idiom is to put the llvm_unreachable after the switch, not in a 
default.  Check out the uses in Type.cpp for examples: some of them are in 
specific cases, saying those cases are illlegal due to some precondition, but 
most of them are just after the switch saying that the switch is covered.  (The 
semantics of switch in C are that switch values with no corresponding case just 
skip the whole switch unless there's a default.)



Comment at: include/clang/Basic/SyncScope.h:89
+  static std::unique_ptr create(LangOptions );
+};
+

The previous design of this header seems more appropriate to me.  Clang defines 
a builtin with standardized argument values for all targets; we're not trying 
to allow targets to customize the arguments taken by this builtin, at least not 
yet.  The sync-scope argument values are the values from this enum. Because 
they are potentially exposed in source programs, they cannot be lightly changed 
and re-ordered; that is, they are ABI.  There is a second level of ABI, which 
is the set of values demanded by target runtime functions.  It is IRGen's 
responsibility to turn the SyncScope values into those values when generating a 
call to the runtime; that mapping does not need to be exposed here in the AST.



Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:

Please correct me if I'm wrong, but I believe you don't get to define the "ABI 
for OpenCL" any more than I get to define the "ABI for C".  You're defining the 
ABI for your specific OpenCL implementation, but Clang might reasonably support 
other implementations with their own ABIs.  So this name is inappropriate.

Now, if I understand how SPIR works, SPIR does require some sort of stable 
lowering of these atomic operations in order to ensure that the resulting LLVM 
IR can be ingested into an arbitrary implementation.  (Ot at least that was 
true prior to SPIR V?)  Therefore, SPIR needs to specify a lowering for these 
atomic operations, and that probably includes ABI values for specific 
sync-scopes.  But the appropriate name for that would be the "SPIR ABI", not 
the "OpenCL ABI".  And, of course, you would need to be sure that you're 
implementing the lowering that SPIR actually wants.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Tony Tye via Phabricator via cfe-commits]

t-tye accepted this revision.
t-tye added a comment.

LGTM


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 110904.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

Refactor to introduce classes AtomicScopeABI and AtomicScopeOpenCLABI for easy 
extension to other languages.


https://reviews.llvm.org/D36580

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/SyncScope.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGAtomic.cpp
  lib/Frontend/InitPreprocessor.cpp
  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
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -14,7 +14,6 @@
 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,
@@ -81,6 +80,13 @@
 }
 
 void memory_checks(atomic_int *Ap, int *p, int val) {
+  // non-integer memory order argument is casted to integer type.
+  (void)__opencl_atomic_load(Ap, 1.0f, memory_scope_work_group);
+  float forder;
+  (void)__opencl_atomic_load(Ap, forder, memory_scope_work_group);
+  struct S s;
+  (void)__opencl_atomic_load(Ap, s, memory_scope_work_group); // expected-error {{passing 'struct S' to parameter of incompatible type 'int'}}
+
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_acquire, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_consume, memory_scope_work_group); // expected-error {{use of undeclared identifier 'memory_order_consume'}}
@@ -151,8 +157,15 @@
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_device);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_all_svm_devices);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_sub_group);
-  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope); // expected-error{{non-constant synchronization scope argument to atomic operation is not supported}}
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, 10);//expected-error{{synchronization scope argument to atomic operation is invalid}}
+
+  // non-integer memory scope is casted to integer type.
+  float fscope;
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, 1.0f);
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, fscope);
+  struct S s;
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, s); //expected-error{{passing 'struct S' to parameter of incompatible type 'int'}}
 }
 
 void nullPointerWarning(atomic_int *Ap, int *p, int val) {
Index: test/CodeGenOpenCL/atomic-ops.cl
===
--- test/CodeGenOpenCL/atomic-ops.cl
+++ test/CodeGenOpenCL/atomic-ops.cl
@@ -52,6 +52,81 @@
   return __opencl_atomic_compare_exchange_strong(i, , 1, memory_order_acquire, memory_order_acquire, memory_scope_work_group);
 }
 
+void fi5(atomic_int *i, int scope) {
+  // CHECK-LABEL: @fi5
+  // CHECK: switch i32 %{{.*}}, label %opencl_allsvmdevices [
+  // CHECK-NEXT: i32 1, label %opencl_workgroup
+  // CHECK-NEXT: i32 2, label %opencl_device
+  // CHECK-NEXT: i32 4, label %opencl_subgroup
+  // CHECK-NEXT: ]
+  // CHECK: opencl_workgroup:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_device:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_allsvmdevices:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_subgroup:
+  // CHECK: %5 = load atomic i32, i32 addrspace(4)* %0 syncscope("subgroup") seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: atomic.scope.continue:
+  int x = __opencl_atomic_load(i, memory_order_seq_cst, scope);
+}
+
+void fi6(atomic_int *i, int order, int scope) {
+  // CHECK-LABEL: @fi6
+  // CHECK: switch i32 %{{.*}}, label %monotonic [
+  // CHECK-NEXT: i32 1, label %acquire
+  // CHECK-NEXT: i32 2, label %acquire
+  // CHECK-NEXT: i32 5, label %seqcst
+  // CHECK-NEXT: ]
+  // CHECK: monotonic:
+  // CHECK: switch i32 %{{.*}}, label %[[MON_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[MON_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[MON_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[MON_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: acquire:
+  // CHECK: switch i32 %{{.*}}, label %[[ACQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[ACQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[ACQ_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[ACQ_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: seqcst:
+  // CHECK: switch i32 %2, label %[[SEQ_ALL:.*]] [
+  // CHECK-NEXT: i32 

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 6 inline comments as done.
yaxunl added inline comments.



Comment at: lib/CodeGen/CGAtomic.cpp:678
+  auto  = CGF.Builder;
+  auto Scopes = getAllLanguageSyncScopes();
+  llvm::DenseMap BB;

t-tye wrote:
> yaxunl wrote:
> > t-tye wrote:
> > > Should only the scopes that apply to the language be returned otherwise 
> > > will be generating code for invalid (possibly duplicate ABI) values?
> > getAllLanguageSyncScopes() only returns scope values for current language. 
> > I will rename it to getRuntimeSyncScopeValuesForCurrentLanguage() to avoid 
> > confusing.
> Curretly getAllLanguageSyncScopes does not take a LangOpt so not sure how it 
> knows what the language is, and did not see it checking that the language is 
> OpenCL internally. For non-OpenCL languages do they still have to support 
> system scope?
For now I just assume all languages use OpenCL memory scope ABI. If a language 
has its own memory scope ABI it can be added later.



Comment at: lib/Frontend/InitPreprocessor.cpp:582
   // The values should match clang SyncScope enum.
-  assert(static_cast(SyncScope::OpenCLWorkGroup) == 1 &&
- static_cast(SyncScope::OpenCLDevice) == 2 &&
- static_cast(SyncScope::OpenCLAllSVMDevices) == 3 &&
- static_cast(SyncScope::OpenCLSubGroup) == 4);
+  assert(static_cast(OpenCLMemoryScope::WorkGroup) == 1 &&
+ static_cast(OpenCLMemoryScope::Device) == 2 &&

t-tye wrote:
> Could this be static_assert?
Will do.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: lib/CodeGen/CGAtomic.cpp:678
+  auto  = CGF.Builder;
+  auto Scopes = getAllLanguageSyncScopes();
+  llvm::DenseMap BB;

yaxunl wrote:
> t-tye wrote:
> > Should only the scopes that apply to the language be returned otherwise 
> > will be generating code for invalid (possibly duplicate ABI) values?
> getAllLanguageSyncScopes() only returns scope values for current language. I 
> will rename it to getRuntimeSyncScopeValuesForCurrentLanguage() to avoid 
> confusing.
Curretly getAllLanguageSyncScopes does not take a LangOpt so not sure how it 
knows what the language is, and did not see it checking that the language is 
OpenCL internally. For non-OpenCL languages do they still have to support 
system scope?


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 8 inline comments as done.
yaxunl added inline comments.



Comment at: include/clang/Basic/SyncScope.h:56
 /// enums in opencl-c.h.
-enum class SyncScope {
-  OpenCLWorkGroup = 1,
-  OpenCLDevice = 2,
-  OpenCLAllSVMDevices = 3,
-  OpenCLSubGroup = 4,
+enum class OpenCLMemoryScope {
+  WorkGroup = 1,

t-tye wrote:
> AtomicScopeOpenCLABI  to mirror AtomicOrderingCABI?
Will do.



Comment at: include/clang/Basic/SyncScope.h:94
+/// and return a language-specific value.
+inline bool isValidLanguageSyncScope(unsigned Scope) {
+  return Scope >= static_cast(OpenCLMemoryScope::WorkGroup) &&

t-tye wrote:
> Should this take a LangOpt since different languages may use different value 
> ABIs?
Although currently this function does not use LangOpt, but I agree it may be a 
good idea to make it future proof. Will add it and update comment.



Comment at: lib/CodeGen/CGAtomic.cpp:678
+  auto  = CGF.Builder;
+  auto Scopes = getAllLanguageSyncScopes();
+  llvm::DenseMap BB;

t-tye wrote:
> Should only the scopes that apply to the language be returned otherwise will 
> be generating code for invalid (possibly duplicate ABI) values?
getAllLanguageSyncScopes() only returns scope values for current language. I 
will rename it to getRuntimeSyncScopeValuesForCurrentLanguage() to avoid 
confusing.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 110863.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.

Revised by John's and Tony's comments.


https://reviews.llvm.org/D36580

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/SyncScope.h
  lib/CodeGen/CGAtomic.cpp
  lib/Frontend/InitPreprocessor.cpp
  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
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -14,7 +14,6 @@
 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,
@@ -81,6 +80,13 @@
 }
 
 void memory_checks(atomic_int *Ap, int *p, int val) {
+  // non-integer memory order argument is casted to integer type.
+  (void)__opencl_atomic_load(Ap, 1.0f, memory_scope_work_group);
+  float forder;
+  (void)__opencl_atomic_load(Ap, forder, memory_scope_work_group);
+  struct S s;
+  (void)__opencl_atomic_load(Ap, s, memory_scope_work_group); // expected-error {{passing 'struct S' to parameter of incompatible type 'int'}}
+
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_acquire, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_consume, memory_scope_work_group); // expected-error {{use of undeclared identifier 'memory_order_consume'}}
@@ -151,8 +157,15 @@
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_device);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_all_svm_devices);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_sub_group);
-  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope); // expected-error{{non-constant synchronization scope argument to atomic operation is not supported}}
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, 10);//expected-error{{synchronization scope argument to atomic operation is invalid}}
+
+  // non-integer memory scope is casted to integer type.
+  float fscope;
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, 1.0f);
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, fscope);
+  struct S s;
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, s); //expected-error{{passing 'struct S' to parameter of incompatible type 'int'}}
 }
 
 void nullPointerWarning(atomic_int *Ap, int *p, int val) {
Index: test/CodeGenOpenCL/atomic-ops.cl
===
--- test/CodeGenOpenCL/atomic-ops.cl
+++ test/CodeGenOpenCL/atomic-ops.cl
@@ -52,6 +52,81 @@
   return __opencl_atomic_compare_exchange_strong(i, , 1, memory_order_acquire, memory_order_acquire, memory_scope_work_group);
 }
 
+void fi5(atomic_int *i, int scope) {
+  // CHECK-LABEL: @fi5
+  // CHECK: switch i32 %{{.*}}, label %opencl_allsvmdevices [
+  // CHECK-NEXT: i32 1, label %opencl_workgroup
+  // CHECK-NEXT: i32 2, label %opencl_device
+  // CHECK-NEXT: i32 4, label %opencl_subgroup
+  // CHECK-NEXT: ]
+  // CHECK: opencl_workgroup:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_device:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_allsvmdevices:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_subgroup:
+  // CHECK: %5 = load atomic i32, i32 addrspace(4)* %0 syncscope("subgroup") seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: atomic.scope.continue:
+  int x = __opencl_atomic_load(i, memory_order_seq_cst, scope);
+}
+
+void fi6(atomic_int *i, int order, int scope) {
+  // CHECK-LABEL: @fi6
+  // CHECK: switch i32 %{{.*}}, label %monotonic [
+  // CHECK-NEXT: i32 1, label %acquire
+  // CHECK-NEXT: i32 2, label %acquire
+  // CHECK-NEXT: i32 5, label %seqcst
+  // CHECK-NEXT: ]
+  // CHECK: monotonic:
+  // CHECK: switch i32 %{{.*}}, label %[[MON_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[MON_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[MON_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[MON_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: acquire:
+  // CHECK: switch i32 %{{.*}}, label %[[ACQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[ACQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[ACQ_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[ACQ_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: seqcst:
+  // CHECK: switch i32 %2, label %[[SEQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[SEQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[SEQ_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[SEQ_SUB:.*]]
+  // 

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[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:46
+  Scopes.push_back(SyncScope::OpenCLSubGroup);
+  return Scopes;
+}

rjmccall wrote:
> t-tye wrote:
> > Should there be an assert/static_assert in case SyncScope enum grows due to 
> > other languages?
> It makes sense to add a 'last' enumerator to SyncScope and do a static_assert 
> here that last == OpenCLSubGroup.
Thanks. Will do.



Comment at: include/clang/Basic/SyncScope.h:47
+  return Scopes;
+}
+

rjmccall wrote:
> You could just return an ArrayRef to a static const array.
Thanks. Will do.



Comment at: include/clang/Basic/SyncScope.h:59
+return "opencl_subgroup";
+  }
+}

rjmccall wrote:
> t-tye wrote:
> > Should there be a default/assert/static_assert to allow SyncScope enum to 
> > grow due to other languages?
> There will already be a warning from -Wswitch about this, which should be 
> sufficient.
> 
> But we do often add llvm_unreachable after switches like this.
I tried adding

```
default:
llvm_unreachable("Invalid sync scope");
```

However, it results in error:

```
 error: default label in switch which covers all enumeration values 
[-Werror,-Wcovered-switch-default]
```
I guess I'd better leave it as it is since we already have -Wswitch.



Comment at: lib/CodeGen/CGAtomic.cpp:687
+
+  auto *SC = Builder.CreateIntCast(Scope, Builder.getInt32Ty(), false);
+  // If unsupported sync scope is encountered at run time, assume default sync

rjmccall wrote:
> Does Sema not coerce the argument to int?  It really should, and then you can 
> just rely on that here.  (You should use CGF.IntTy if you do this, though, in 
> case you're on a target with a non-32-bit int.)
It is coerced to `Context.IntTy`. I will add some tests for non-integral 
order/scope argument.



Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+  SI->addCase(Builder.getInt32(static_cast(S)), B);
+

rjmccall wrote:
> t-tye wrote:
> > rjmccall wrote:
> > > t-tye wrote:
> > > > Is it documented in the SyncScope enum that the enumerator values are 
> > > > in fact the values used for source language runtime values? Seems if 
> > > > other languages want to use scopes they may may have a different 
> > > > ordering. That would imply that there would be a function to map a 
> > > > SyncScope value to the value used by the source language. For OpenCL 
> > > > the mapping is identity.
> > > > 
> > > > The memory ordering has the isValidAtomicOrderingCABI() that does a 
> > > > similar thing.
> > > The values in the SyncScope enum are the source language values.  We 
> > > already have a step to translate them into LLVM values when we generate a 
> > > native LLVM construct.  To the extent that we call into a runtime 
> > > instead, none of that code has been written to be runtime-agnostic at 
> > > all, and I've been assuming that we're basically okay with that, at least 
> > > for now.
> > That sounds reasonable to me. When/if another language comes along the task 
> > of mapping the multiple ABIs can be done then. Still wanted to make sure it 
> > is clear that the enum values in the SyncScope enum are documented as BEING 
> > the OpenCL runtime ABI values and not some arbitrary list as in other enums 
> > such as the address space enum. 
> They are documented as being the values expected by the builtins, so they 
> already can't be arbitrarily changed.
> 
> Now, the values expected by the builtin were chosen to match this specific 
> runtime, but we had to choose something, and matching a runtime isn't the 
> worst way of doing that.  But now that I think about it, those values seem to 
> be rather sub-optimal because they don't start at 0, which means that things 
> like this switch will be less efficient than they could be.  And I think the 
> AST and IRGen would benefit from needing to renumber values; it will make the 
> code clearer, and it'll help protect against people adding values to this 
> enum just because those values are used by the runtime, i.e. without fully 
> realizing that they're expanding a user-facing feature.  So let's go ahead 
> and renumber the values in SyncScope to start at 0 and then re-map them in 
> IRGen.
Will do. Thanks.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+  SI->addCase(Builder.getInt32(static_cast(S)), B);
+

t-tye wrote:
> rjmccall wrote:
> > t-tye wrote:
> > > Is it documented in the SyncScope enum that the enumerator values are in 
> > > fact the values used for source language runtime values? Seems if other 
> > > languages want to use scopes they may may have a different ordering. That 
> > > would imply that there would be a function to map a SyncScope value to 
> > > the value used by the source language. For OpenCL the mapping is identity.
> > > 
> > > The memory ordering has the isValidAtomicOrderingCABI() that does a 
> > > similar thing.
> > The values in the SyncScope enum are the source language values.  We 
> > already have a step to translate them into LLVM values when we generate a 
> > native LLVM construct.  To the extent that we call into a runtime instead, 
> > none of that code has been written to be runtime-agnostic at all, and I've 
> > been assuming that we're basically okay with that, at least for now.
> That sounds reasonable to me. When/if another language comes along the task 
> of mapping the multiple ABIs can be done then. Still wanted to make sure it 
> is clear that the enum values in the SyncScope enum are documented as BEING 
> the OpenCL runtime ABI values and not some arbitrary list as in other enums 
> such as the address space enum. 
They are documented as being the values expected by the builtins, so they 
already can't be arbitrarily changed.

Now, the values expected by the builtin were chosen to match this specific 
runtime, but we had to choose something, and matching a runtime isn't the worst 
way of doing that.  But now that I think about it, those values seem to be 
rather sub-optimal because they don't start at 0, which means that things like 
this switch will be less efficient than they could be.  And I think the AST and 
IRGen would benefit from needing to renumber values; it will make the code 
clearer, and it'll help protect against people adding values to this enum just 
because those values are used by the runtime, i.e. without fully realizing that 
they're expanding a user-facing feature.  So let's go ahead and renumber the 
values in SyncScope to start at 0 and then re-map them in IRGen.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Tony Tye via Phabricator via cfe-commits]

t-tye accepted this revision.
t-tye added a comment.
This revision is now accepted and ready to land.

LGTM other than suggested documentation and static_assert/unreachable comments.




Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+  SI->addCase(Builder.getInt32(static_cast(S)), B);
+

rjmccall wrote:
> t-tye wrote:
> > Is it documented in the SyncScope enum that the enumerator values are in 
> > fact the values used for source language runtime values? Seems if other 
> > languages want to use scopes they may may have a different ordering. That 
> > would imply that there would be a function to map a SyncScope value to the 
> > value used by the source language. For OpenCL the mapping is identity.
> > 
> > The memory ordering has the isValidAtomicOrderingCABI() that does a similar 
> > thing.
> The values in the SyncScope enum are the source language values.  We already 
> have a step to translate them into LLVM values when we generate a native LLVM 
> construct.  To the extent that we call into a runtime instead, none of that 
> code has been written to be runtime-agnostic at all, and I've been assuming 
> that we're basically okay with that, at least for now.
That sounds reasonable to me. When/if another language comes along the task of 
mapping the multiple ABIs can be done then. Still wanted to make sure it is 
clear that the enum values in the SyncScope enum are documented as BEING the 
OpenCL runtime ABI values and not some arbitrary list as in other enums such as 
the address space enum. 


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Basic/SyncScope.h:46
+  Scopes.push_back(SyncScope::OpenCLSubGroup);
+  return Scopes;
+}

t-tye wrote:
> Should there be an assert/static_assert in case SyncScope enum grows due to 
> other languages?
It makes sense to add a 'last' enumerator to SyncScope and do a static_assert 
here that last == OpenCLSubGroup.



Comment at: include/clang/Basic/SyncScope.h:59
+return "opencl_subgroup";
+  }
+}

t-tye wrote:
> Should there be a default/assert/static_assert to allow SyncScope enum to 
> grow due to other languages?
There will already be a warning from -Wswitch about this, which should be 
sufficient.

But we do often add llvm_unreachable after switches like this.



Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+  SI->addCase(Builder.getInt32(static_cast(S)), B);
+

t-tye wrote:
> Is it documented in the SyncScope enum that the enumerator values are in fact 
> the values used for source language runtime values? Seems if other languages 
> want to use scopes they may may have a different ordering. That would imply 
> that there would be a function to map a SyncScope value to the value used by 
> the source language. For OpenCL the mapping is identity.
> 
> The memory ordering has the isValidAtomicOrderingCABI() that does a similar 
> thing.
The values in the SyncScope enum are the source language values.  We already 
have a step to translate them into LLVM values when we generate a native LLVM 
construct.  To the extent that we call into a runtime instead, none of that 
code has been written to be runtime-agnostic at all, and I've been assuming 
that we're basically okay with that, at least for now.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: include/clang/Basic/SyncScope.h:46
+  Scopes.push_back(SyncScope::OpenCLSubGroup);
+  return Scopes;
+}

Should there be an assert/static_assert in case SyncScope enum grows due to 
other languages?



Comment at: include/clang/Basic/SyncScope.h:59
+return "opencl_subgroup";
+  }
+}

Should there be a default/assert/static_assert to allow SyncScope enum to grow 
due to other languages?



Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+  SI->addCase(Builder.getInt32(static_cast(S)), B);
+

Is it documented in the SyncScope enum that the enumerator values are in fact 
the values used for source language runtime values? Seems if other languages 
want to use scopes they may may have a different ordering. That would imply 
that there would be a function to map a SyncScope value to the value used by 
the source language. For OpenCL the mapping is identity.

The memory ordering has the isValidAtomicOrderingCABI() that does a similar 
thing.


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Basic/SyncScope.h:47
+  return Scopes;
+}
+

You could just return an ArrayRef to a static const array.



Comment at: lib/CodeGen/CGAtomic.cpp:687
+
+  auto *SC = Builder.CreateIntCast(Scope, Builder.getInt32Ty(), false);
+  // If unsupported sync scope is encountered at run time, assume default sync

Does Sema not coerce the argument to int?  It really should, and then you can 
just rely on that here.  (You should use CGF.IntTy if you do this, though, in 
case you're on a target with a non-32-bit int.)


https://reviews.llvm.org/D36580



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

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.

https://reviews.llvm.org/D36580

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/SyncScope.h
  lib/CodeGen/CGAtomic.cpp
  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
===
--- test/SemaOpenCL/atomic-ops.cl
+++ test/SemaOpenCL/atomic-ops.cl
@@ -151,7 +151,7 @@
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_device);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_all_svm_devices);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_sub_group);
-  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope); // expected-error{{non-constant synchronization scope argument to atomic operation is not supported}}
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, 10);//expected-error{{synchronization scope argument to atomic operation is invalid}}
 }
 
Index: test/CodeGenOpenCL/atomic-ops.cl
===
--- test/CodeGenOpenCL/atomic-ops.cl
+++ test/CodeGenOpenCL/atomic-ops.cl
@@ -52,6 +52,81 @@
   return __opencl_atomic_compare_exchange_strong(i, , 1, memory_order_acquire, memory_order_acquire, memory_scope_work_group);
 }
 
+void fi5(atomic_int *i, int scope) {
+  // CHECK-LABEL: @fi5
+  // CHECK: switch i32 %{{.*}}, label %opencl_allsvmdevices [
+  // CHECK-NEXT: i32 1, label %opencl_workgroup
+  // CHECK-NEXT: i32 2, label %opencl_device
+  // CHECK-NEXT: i32 4, label %opencl_subgroup
+  // CHECK-NEXT: ]
+  // CHECK: opencl_workgroup:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_device:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") seq_cst
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_allsvmdevices:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: opencl_subgroup:
+  // CHECK: %5 = load atomic i32, i32 addrspace(4)* %0 syncscope("subgroup") seq_cst, align 4
+  // CHECK: br label %atomic.scope.continue
+  // CHECK: atomic.scope.continue:
+  int x = __opencl_atomic_load(i, memory_order_seq_cst, scope);
+}
+
+void fi6(atomic_int *i, int order, int scope) {
+  // CHECK-LABEL: @fi6
+  // CHECK: switch i32 %{{.*}}, label %monotonic [
+  // CHECK-NEXT: i32 1, label %acquire
+  // CHECK-NEXT: i32 2, label %acquire
+  // CHECK-NEXT: i32 5, label %seqcst
+  // CHECK-NEXT: ]
+  // CHECK: monotonic:
+  // CHECK: switch i32 %{{.*}}, label %[[MON_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[MON_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[MON_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[MON_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: acquire:
+  // CHECK: switch i32 %{{.*}}, label %[[ACQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[ACQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[ACQ_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[ACQ_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: seqcst:
+  // CHECK: switch i32 %2, label %[[SEQ_ALL:.*]] [
+  // CHECK-NEXT: i32 1, label %[[SEQ_WG:.*]]
+  // CHECK-NEXT: i32 2, label %[[SEQ_DEV:.*]]
+  // CHECK-NEXT: i32 4, label %[[SEQ_SUB:.*]]
+  // CHECK-NEXT: ]
+  // CHECK: [[MON_WG]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") monotonic
+  // CHECK: [[MON_DEV]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") monotonic
+  // CHECK: [[MON_ALL]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} monotonic
+  // CHECK: [[MON_SUB]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("subgroup") monotonic
+  // CHECK: [[ACQ_WG]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") acquire
+  // CHECK: [[ACQ_DEV]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") acquire
+  // CHECK: [[ACQ_ALL]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} acquire
+  // CHECK: [[ACQ_SUB]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("subgroup") acquire
+  // CHECK: [[SEQ_WG]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("workgroup") seq_cst
+  // CHECK: [[SEQ_DEV]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("agent") seq_cst
+  // CHECK: [[SEQ_ALL]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} seq_cst
+  // CHECK: [[SEQ_SUB]]:
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{.*}} syncscope("subgroup") seq_cst
+  int x = __opencl_atomic_load(i, order, scope);
+}
+
 float ff1(global atomic_float *d) {
   // CHECK-LABEL: @ff1
   // CHECK: load atomic i32, i32 addrspace(1)* {{.*}} syncscope("workgroup") monotonic
Index: