[PATCH] D28080: [Docs][OpenCL] Added OpenCL feature description to user manual.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: docs/UsersManual.rst:2065
+
+ $ clang -cc1 -triple spir64-unknown-unknown -cl-ext=-cl_khr_fp64 test.cl
+

pekka.jaaskelainen wrote:
> Anastasia wrote:
> > pekka.jaaskelainen wrote:
> > > Is this correct? I cannot make it work:
> > > 
> > > ```
> > > ~/local/stow/llvm-4.0-unpatched-Debug/bin/clang -Xclang 
> > > -finclude-default-header -emit-llvm -cc1 -triple spir64-unknown-unknown 
> > > kernel/test_halfs.cl -c -S -o -
> > > clang-4.0: error: unknown argument: '-cc1'
> > > clang-4.0: error: unknown argument: '-triple'
> > > clang-4.0: error: no such file or directory: 'spir64-unknown-unknown'
> > > ```
> > > 
> > > -target works instead. (But reveals another issue, there's no printf() 
> > > declaration, should I file a bug?)
> > > 
> > > ```
> > > ~/local/stow/llvm-4.0-unpatched-Debug/bin/clang -Xclang 
> > > -finclude-default-header -emit-llvm -target spir64-unknown-unknown 
> > > kernel/test_halfs.cl -c -S -o -
> > > kernel/test_halfs.cl:10:9: warning: implicit declaration of function 
> > > 'printf' is invalid in C99 [-Wimplicit-function-declaration]
> > > printf("max(a,b)[0] type=uint2 a=0x15b348c9 b=0xf88e7d07 
> > > want=0xf88e7d07 got=%x\n", max_[0]);
> > > ^
> > > kernel/test_halfs.cl:10:9: error: function with no prototype cannot use 
> > > the spir_function calling convention
> > > 1 warning and 1 error generated.
> > > ```
> > > 
> > > With 3.9 that kernel which calls printf() passes:
> > > ```
> > > /local/stow/llvm-3.9-debug/bin/clang -Xclang -finclude-default-header 
> > > -emit-llvm -target spir64-unknown-unknown kernel/test_halfs.cl -c -S -o -
> > > ; ModuleID = 'kernel/test_halfs.cl'
> > > source_filename = "kernel/test_halfs.cl"
> > > target datalayout = 
> > > "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
> > > target triple = "spir64-unknown-unknown"
> > > 
> > > ```
> > > 
> > > 
> > > 
> > It should be -triple with -cc1 and -target without.
> > 
> >   clang -cc1 -triple ...
> >   clang -target ...
> > 
> > We have disabled the C99 builtin functions as per spec v1.2 s6.9.f, but the 
> > OpenCL builtins should be available via the header now.
> > 
> > As I can see declaration of printf is in the header under CL1.2 and higher.
> > 
> > Could you try this command instead:
> >   clang -std=CL1.2 test.cl -Xclang -finclude-default-header
> > 
> Yes, std=CL1.2 helped, good point! But -cc1 -triple doesn't work (unknown 
> argument). However -target works. This is with Clang 4.0.
I think you pass something before -cc1. -cc1 option must be the first argument 
to the clang executable, otherwise it won't work.



https://reviews.llvm.org/D28080



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

[PATCH] D27099: [OpenCL] Prohibit using reserve_id_t in program scope.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288126: [OpenCL] Prohibit using reserve_id_t in program 
scope. (authored by bader).

Changed prior to commit:
  https://reviews.llvm.org/D27099?vs=79508=79525#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27099

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/test/SemaOpenCL/event_t.cl
  cfe/trunk/test/SemaOpenCL/invalid-clk-events-cl2.0.cl
  cfe/trunk/test/SemaOpenCL/invalid-pipes-cl2.0.cl

Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -5909,32 +5909,31 @@
 return nullptr;
   }
 
-  // OpenCL v2.0 s6.9.b - Image type can only be used as a function argument.
-  // OpenCL v2.0 s6.13.16.1 - Pipe type can only be used as a function
-  // argument.
-  if (getLangOpts().OpenCL && (R->isImageType() || R->isPipeType())) {
-Diag(D.getIdentifierLoc(),
- diag::err_opencl_type_can_only_be_used_as_function_parameter)
-<< R;
-D.setInvalidType();
-return nullptr;
-  }
-
-  DeclSpec::SCS SCSpec = D.getDeclSpec().getStorageClassSpec();
-  StorageClass SC = StorageClassSpecToVarDeclStorageClass(D.getDeclSpec());
-
-  // dllimport globals without explicit storage class are treated as extern. We
-  // have to change the storage class this early to get the right DeclContext.
-  if (SC == SC_None && !DC->isRecord() &&
-  hasParsedAttr(S, D, AttributeList::AT_DLLImport) &&
-  !hasParsedAttr(S, D, AttributeList::AT_DLLExport))
-SC = SC_Extern;
+  if (getLangOpts().OpenCL) {
+// OpenCL v2.0 s6.9.b - Image type can only be used as a function argument.
+// OpenCL v2.0 s6.13.16.1 - Pipe type can only be used as a function
+// argument.
+if (R->isImageType() || R->isPipeType()) {
+  Diag(D.getIdentifierLoc(),
+   diag::err_opencl_type_can_only_be_used_as_function_parameter)
+  << R;
+  D.setInvalidType();
+  return nullptr;
+}
 
-  DeclContext *OriginalDC = DC;
-  bool IsLocalExternDecl = SC == SC_Extern &&
-   adjustContextForLocalExternDecl(DC);
+// OpenCL v1.2 s6.9.r:
+// The event type cannot be used to declare a program scope variable.
+// OpenCL v2.0 s6.9.q:
+// The clk_event_t and reserve_id_t types cannot be declared in program scope.
+if (NULL == S->getParent()) {
+  if (R->isReserveIDT() || R->isClkEventT() || R->isEventT()) {
+Diag(D.getIdentifierLoc(),
+ diag::err_invalid_type_for_program_scope_var) << R;
+D.setInvalidType();
+return nullptr;
+  }
+}
 
-  if (getLangOpts().OpenCL) {
 // OpenCL v1.0 s6.8.a.3: Pointers to functions are not allowed.
 QualType NR = R;
 while (NR->isPointerType()) {
@@ -5954,8 +5953,40 @@
 D.setInvalidType();
   }
 }
+
+// OpenCL v1.2 s6.9.b p4:
+// The sampler type cannot be used with the __local and __global address
+// space qualifiers.
+if (R->isSamplerT() && (R.getAddressSpace() == LangAS::opencl_local ||
+  R.getAddressSpace() == LangAS::opencl_global)) {
+  Diag(D.getIdentifierLoc(), diag::err_wrong_sampler_addressspace);
+}
+
+// OpenCL v1.2 s6.9.r:
+// The event type cannot be used with the __local, __constant and __global
+// address space qualifiers.
+if (R->isEventT()) {
+  if (R.getAddressSpace()) {
+Diag(D.getLocStart(), diag::err_event_t_addr_space_qual);
+D.setInvalidType();
+  }
+}
   }
 
+  DeclSpec::SCS SCSpec = D.getDeclSpec().getStorageClassSpec();
+  StorageClass SC = StorageClassSpecToVarDeclStorageClass(D.getDeclSpec());
+
+  // dllimport globals without explicit storage class are treated as extern. We
+  // have to change the storage class this early to get the right DeclContext.
+  if (SC == SC_None && !DC->isRecord() &&
+  hasParsedAttr(S, D, AttributeList::AT_DLLImport) &&
+  !hasParsedAttr(S, D, AttributeList::AT_DLLExport))
+SC = SC_Extern;
+
+  DeclContext *OriginalDC = DC;
+  bool IsLocalExternDecl = SC == SC_Extern &&
+   adjustContextForLocalExternDecl(DC);
+
   if (SCSpec == DeclSpec::SCS_mutable) {
 // mutable can only appear on non-static class members, so it's always
 // an error here
@@ -5988,32 +6019,6 @@
 }
   }
 
-  if (getLangOpts().OpenCL) {
-// OpenCL v1.2 s6.9.b p4:
-// The sampler type cannot be used with the __local and __global address
-// space qualifiers.
-if (R->isSamplerT() && (R.getAddressSpace() == LangAS::opencl_local ||
-  R.getAddressSpace() == LangAS::opencl_global)) {
-  Diag(D.getIdentifierLoc(), diag::err_wrong_sampler_addressspace);
-}
-
-// OpenCL 1.2 spec, p6.9 r:
-// The event type cannot be used to declare a program scope variable.
-// The event type cannot be used with 

[PATCH] D27403: [OpenCL] Added a LIT test for ensuring address space mangling is done the same both in OpenCL1.2 and OpenCL2.0.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@echuraev, please, request commit access as described here: 
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access


Repository:
  rL LLVM

https://reviews.llvm.org/D27403



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

[PATCH] D27300: [OpenCL] Fix SPIR version generation.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288890: [OpenCL] Fix SPIR version generation. (authored by 
bader).

Changed prior to commit:
  https://reviews.llvm.org/D27300?vs=80229=80548#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27300

Files:
  cfe/trunk/lib/CodeGen/TargetInfo.cpp
  cfe/trunk/test/CodeGenOpenCL/spir_version.cl


Index: cfe/trunk/test/CodeGenOpenCL/spir_version.cl
===
--- cfe/trunk/test/CodeGenOpenCL/spir_version.cl
+++ cfe/trunk/test/CodeGenOpenCL/spir_version.cl
@@ -13,19 +13,22 @@
 
 // CHECK-SPIR-CL10: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
 // CHECK-SPIR-CL10: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-SPIR-CL10: [[SPIR]] = !{i32 2, i32 0}
+// CHECK-SPIR-CL10: [[SPIR]] = !{i32 1, i32 2}
 // CHECK-SPIR-CL10: [[OCL]] = !{i32 1, i32 0}
-// CHECK-SPIR-CL12: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL12: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-SPIR-CL12: [[SPIR]] = !{i32 2, i32 0}
-// CHECK-SPIR-CL12: [[OCL]] = !{i32 1, i32 2}
-// CHECK-SPIR-CL20: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL20: !opencl.ocl.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL20: [[SPIR]] = !{i32 2, i32 0}
+// CHECK-SPIR-CL12: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL12: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL12: [[VER]] = !{i32 1, i32 2}
 
+// CHECK-SPIR-CL20: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL20: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL20: [[VER]] = !{i32 2, i32 0}
+
+// CHECK-AMDGCN-CL10-NOT: !opencl.spir.version
 // CHECK-AMDGCN-CL10: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
 // CHECK-AMDGCN-CL10: [[OCL]] = !{i32 1, i32 0}
+// CHECK-AMDGCN-CL12-NOT: !opencl.spir.version
 // CHECK-AMDGCN-CL12: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
 // CHECK-AMDGCN-CL12: [[OCL]] = !{i32 1, i32 2}
+// CHECK-AMDGCN-CL20-NOT: !opencl.spir.version
 // CHECK-AMDGCN-CL20: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-AMDGCN-CL20: [[OCL]] = !{i32 2, i32 0}
\ No newline at end of file
+// CHECK-AMDGCN-CL20: [[OCL]] = !{i32 2, i32 0}
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -7801,8 +7801,10 @@
   // SPIR v2.0 s2.12 - The SPIR version used by the module is stored in the
   // opencl.spir.version named metadata.
   llvm::Metadata *SPIRVerElts[] = {
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(Int32Ty, 2)),
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(Int32Ty, 0))};
+  llvm::ConstantAsMetadata::get(
+  llvm::ConstantInt::get(Int32Ty, CGM.getLangOpts().OpenCLVersion / 
100)),
+  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
+  Int32Ty, (CGM.getLangOpts().OpenCLVersion / 100 > 1) ? 0 : 2))};
   llvm::NamedMDNode *SPIRVerMD =
   M.getOrInsertNamedMetadata("opencl.spir.version");
   SPIRVerMD->addOperand(llvm::MDNode::get(Ctx, SPIRVerElts));


Index: cfe/trunk/test/CodeGenOpenCL/spir_version.cl
===
--- cfe/trunk/test/CodeGenOpenCL/spir_version.cl
+++ cfe/trunk/test/CodeGenOpenCL/spir_version.cl
@@ -13,19 +13,22 @@
 
 // CHECK-SPIR-CL10: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
 // CHECK-SPIR-CL10: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-SPIR-CL10: [[SPIR]] = !{i32 2, i32 0}
+// CHECK-SPIR-CL10: [[SPIR]] = !{i32 1, i32 2}
 // CHECK-SPIR-CL10: [[OCL]] = !{i32 1, i32 0}
-// CHECK-SPIR-CL12: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL12: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-SPIR-CL12: [[SPIR]] = !{i32 2, i32 0}
-// CHECK-SPIR-CL12: [[OCL]] = !{i32 1, i32 2}
-// CHECK-SPIR-CL20: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL20: !opencl.ocl.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL20: [[SPIR]] = !{i32 2, i32 0}
+// CHECK-SPIR-CL12: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL12: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL12: [[VER]] = !{i32 1, i32 2}
 
+// CHECK-SPIR-CL20: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL20: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL20: [[VER]] = !{i32 2, i32 0}
+
+// CHECK-AMDGCN-CL10-NOT: !opencl.spir.version
 // CHECK-AMDGCN-CL10: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
 // CHECK-AMDGCN-CL10: [[OCL]] = !{i32 1, i32 0}
+// CHECK-AMDGCN-CL12-NOT: !opencl.spir.version
 // CHECK-AMDGCN-CL12: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
 // CHECK-AMDGCN-CL12: [[OCL]] = !{i32 1, i32 2}
+// CHECK-AMDGCN-CL20-NOT: !opencl.spir.version
 // CHECK-AMDGCN-CL20: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-AMDGCN-CL20: [[OCL]] = !{i32 2, i32 0}
\ No newline at end of file
+// CHECK-AMDGCN-CL20: [[OCL]] = !{i32 2, i32 0}
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp

[PATCH] D27403: [OpenCL] Added a LIT test for ensuring address space mangling is done the same both in OpenCL1.2 and OpenCL2.0.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288891: [OpenCL] Added a LIT test for ensuring address space 
mangling is done the same… (authored by bader).

Changed prior to commit:
  https://reviews.llvm.org/D27403?vs=80401=80549#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27403

Files:
  cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl


Index: cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
===
--- cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
+++ cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
@@ -1,30 +1,44 @@
 // RUN: %clang_cc1 %s -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple %itanium_abi_triple -emit-llvm -o - | 
FileCheck -check-prefix=ASMANG %s
 // RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no 
-triple %itanium_abi_triple -emit-llvm -o - | FileCheck -check-prefix=NOASMANG 
%s
 
+// We check that the address spaces are mangled the same in both version of 
OpenCL
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL2.0 -emit-llvm -o 
- | FileCheck -check-prefix=OCL-20 %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL1.2 -emit-llvm -o 
- | FileCheck -check-prefix=OCL-12 %s
+
 // We can't name this f as private is equivalent to default
 // no specifier given address space so we get multiple definition
 // warnings, but we do want it for comparison purposes.
 __attribute__((overloadable))
 void ff(int *arg) { }
 // ASMANG: @_Z2ffPi
 // NOASMANG: @_Z2ffPi
+// OCL-20-DAG: @_Z2ffPU3AS4i
+// OCL-12-DAG: @_Z2ffPi
 
 __attribute__((overloadable))
 void f(private int *arg) { }
 // ASMANG: @_Z1fPi
 // NOASMANG: @_Z1fPi
+// OCL-20-DAG: @_Z1fPi
+// OCL-12-DAG: @_Z1fPi
 
 __attribute__((overloadable))
 void f(global int *arg) { }
 // ASMANG: @_Z1fPU3AS1i
 // NOASMANG: @_Z1fPU8CLglobali
+// OCL-20-DAG: @_Z1fPU3AS1i
+// OCL-12-DAG: @_Z1fPU3AS1i
 
 __attribute__((overloadable))
 void f(local int *arg) { }
 // ASMANG: @_Z1fPU3AS2i
 // NOASMANG: @_Z1fPU7CLlocali
+// OCL-20-DAG: @_Z1fPU3AS2i
+// OCL-12-DAG: @_Z1fPU3AS2i
 
 __attribute__((overloadable))
 void f(constant int *arg) { }
 // ASMANG: @_Z1fPU3AS3i
 // NOASMANG: @_Z1fPU10CLconstanti
+// OCL-20-DAG: @_Z1fPU3AS3i
+// OCL-12-DAG: @_Z1fPU3AS3i


Index: cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
===
--- cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
+++ cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
@@ -1,30 +1,44 @@
 // RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=yes -triple %itanium_abi_triple -emit-llvm -o - | FileCheck -check-prefix=ASMANG %s
 // RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no -triple %itanium_abi_triple -emit-llvm -o - | FileCheck -check-prefix=NOASMANG %s
 
+// We check that the address spaces are mangled the same in both version of OpenCL
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL2.0 -emit-llvm -o - | FileCheck -check-prefix=OCL-20 %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL1.2 -emit-llvm -o - | FileCheck -check-prefix=OCL-12 %s
+
 // We can't name this f as private is equivalent to default
 // no specifier given address space so we get multiple definition
 // warnings, but we do want it for comparison purposes.
 __attribute__((overloadable))
 void ff(int *arg) { }
 // ASMANG: @_Z2ffPi
 // NOASMANG: @_Z2ffPi
+// OCL-20-DAG: @_Z2ffPU3AS4i
+// OCL-12-DAG: @_Z2ffPi
 
 __attribute__((overloadable))
 void f(private int *arg) { }
 // ASMANG: @_Z1fPi
 // NOASMANG: @_Z1fPi
+// OCL-20-DAG: @_Z1fPi
+// OCL-12-DAG: @_Z1fPi
 
 __attribute__((overloadable))
 void f(global int *arg) { }
 // ASMANG: @_Z1fPU3AS1i
 // NOASMANG: @_Z1fPU8CLglobali
+// OCL-20-DAG: @_Z1fPU3AS1i
+// OCL-12-DAG: @_Z1fPU3AS1i
 
 __attribute__((overloadable))
 void f(local int *arg) { }
 // ASMANG: @_Z1fPU3AS2i
 // NOASMANG: @_Z1fPU7CLlocali
+// OCL-20-DAG: @_Z1fPU3AS2i
+// OCL-12-DAG: @_Z1fPU3AS2i
 
 __attribute__((overloadable))
 void f(constant int *arg) { }
 // ASMANG: @_Z1fPU3AS3i
 // NOASMANG: @_Z1fPU10CLconstanti
+// OCL-20-DAG: @_Z1fPU3AS3i
+// OCL-12-DAG: @_Z1fPU3AS3i
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28080: [Docs][OpenCL] Added OpenCL feature description to user manual.

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.

LGTM.
Thanks!


https://reviews.llvm.org/D28080



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

[PATCH] D28080: [Docs][OpenCL] Added OpenCL feature description to user manual.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Thanks for working on this!
Looks good, except a few pedantic notes.




Comment at: docs/UsersManual.rst:44
 -  :ref:`Objective C++ Language `
+-  :ref:`OpenCL Language `: v1.0, v1.1, v1.2, v2.0.
 

It should and to be perfectly clear we might consider using term "OpenCL C 
kernel language" to avoid confusion with OpenCL host code language or OpenCL 
C++ kernel language.



Comment at: docs/UsersManual.rst:2009
+
+ $ clang test.cl -c -emit-llvm
+

Just for the sake of consistency it would be nice to switch input file name 
with the options:

   clang -c -emit-llvm test.cl



Comment at: docs/UsersManual.rst:2014
+
+Clang currently supports OpenCL standards up to v2.0.
+

I suggest specifying that Clang support OpenCL C kernel language standards up 
to v2.0.
For instance OpenCL v2.1 doesn't introduce new OpenCL C kernel language 
standard - it still uses v2.0. So existing clang is sufficient enough to serve 
as front-end compiler in OpenCL 2.1 driver.



Comment at: docs/UsersManual.rst:2120
+  that can be used across GPU toolchains. The implementation follows `the SPIR
+  specification `_. There are two flavors 
available
+  for 32 and 64 bits.

pekka.jaaskelainen wrote:
> Which version of SPIR is generated?
For -cl-std=CL1.x (where x is 0, 1 or 2), SPIR version is 1.2.
For -cl-std=CL2.0, SPIR version is 2.0.



Comment at: docs/UsersManual.rst:2142-2143
+
+By default Clang will not include standard headers and therefore OpenCL builtin
+functions are unknown. The default CL header is, however, provided in the Clang
+installation and can be enabled by passing the ``-finclude-default-header`` 
flag

Not sure it's worth to mention, but even built-in vector types are defined in 
the opencl-c.h.


https://reviews.llvm.org/D28080



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

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:

> This change seems to modify normal C behavior again. Is there any strong 
> motivation for doing this and if yes could it be done generically with C?


Motivation:

  // Non-portable OpenCL 1.2 code 
  __kernel void foo(global float* out) {
out[get_global_id(0)] = sin(get_global_id(0));
  }

This program compiles fine on OpenCL platform w/o doubles support and fails 
otherwise.
If OpenCL driver supports doubles it provides at least two versions of 'sin' 
built-in math function and compiler will not be able to choose the right one 
for 'size_t' argument.
The goal of this warning is to let OpenCL developer know about potential issues 
with code portability.

This might be not so serious issue for C, since C doesn't support function 
overloading, whereas OpenCL built-in functions library must overload most of 
the functions.


https://reviews.llvm.org/D27334



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

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D27334#613504, @Anastasia wrote:

> In https://reviews.llvm.org/D27334#612858, @bader wrote:
>
> > In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
> >
> > > This change seems to modify normal C behavior again. Is there any strong 
> > > motivation for doing this and if yes could it be done generically with C?
> >
> >
> > Motivation:
> >
> >   // Non-portable OpenCL 1.2 code 
> >   __kernel void foo(global float* out) {
> > out[get_global_id(0)] = sin(get_global_id(0));
> >   }
> >
> >
> > This program compiles fine on OpenCL platform w/o doubles support and fails 
> > otherwise.
> >  If OpenCL driver supports doubles it provides at least two versions of 
> > 'sin' built-in math function and compiler will not be able to choose the 
> > right one for 'size_t' argument.
> >  The goal of this warning is to let OpenCL developer know about potential 
> > issues with code portability.
>
>
> I would argue this improves the portability much as it can also be misleading 
> in some situations (because it refers to a potentially hypothetical problem). 
> For example there can be builtin functions that only have a float parameter 
> (without a double version of it). This is for example the case with 
> read_image functions that take a float coordinate value between 0 and 1. 
> Unfortunately this warning won't be triggered on read_image functions because 
> there is an overload candidate with an int type of the same parameter too. 
> But we can't exclude this situations to appear in the future or from some 
> vendor extensions or even custom OpenCL code.


As much as any other warning it's not always means that there is an error in 
the code. It just means that developer should inspect the construction 
triggering a warning.
Passing integer value to a function with floating point parameters is not 
always an error, but some times it might be so.
Do you suggest dropping the diagnostics at all or changing the diagnostics 
message?


https://reviews.llvm.org/D27334



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

[PATCH] D31397: [Bug 25404] Fix crash on typedef in OpenCL 2.0

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Anastasia, could you generate patch with full context, please?


https://reviews.llvm.org/D31397



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

[PATCH] D31397: [Bug 25404] Fix crash on typedef in OpenCL 2.0

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:2157
   if (getDiagnostics().getSuppressSystemWarnings() &&
-  (Context.getSourceManager().isInSystemHeader(Old->getLocation()) ||
+  // Some standard types are defined implicitly in Clang (e.g. OpenCL).
+  (Old->isImplicit() || New->isImplicit() ||

Anastasia wrote:
> Anastasia wrote:
> > ahatanak wrote:
> > > Is it necessary to check whether New is implicit? I was just wondering 
> > > when or how an implicit definition would redefine a typedef.
> > I had a thought on it too, and I am not sure actually. This can happen if 
> > we implicitly define something from the standard headers. But I believe the 
> > Sema initialization should always happen before parsing the standard header 
> > or even loading them from the PCH. So I guess this shouldn't ever happen 
> > really? Perhaps, I should just remove this?
> Actually in case of implicit typedefs we don't seem to follow this program 
> path at all. So I am removing this.
So something like this will also work?
```
typedef float float4 __attribute((ext_vector_type(4)));
typedef float4 atomic_int;
```




Comment at: test/SemaOpenCL/types.cl:6
+// Check redefinition of standard types
+typedef atomic_int atomic_flag;

Can we check that -Wtypedef-redefinition will emit a warning for this 
expression?
This typedef seems to be unnecessary since clang implicitly defines atomic_flag 
for OpenCL. User is not supposed to re-define it, so warning would be helpful 
here.


https://reviews.llvm.org/D31397



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

[PATCH] D31397: [Bug 25404] Fix crash on typedef in OpenCL 2.0

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

I think my topics are not related to the bug 25404 and can be handled 
separately.
Thanks!


https://reviews.llvm.org/D31397



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

[PATCH] D28136: [OpenCL] Implement as_type operator as alias of __builtin_astype.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

> From all the above arguments, I feel like the right approach would be to 
> implement it as Clang builtin which closely matches the operator semantic in 
> my opinion. We could of course reuse the implementation of  __bultin_astype 
> to avoid unnecessary extra work and code duplication.
> 
> Using the macro seems to me more like a workaround solution and overloaded 
> functions don't seem to be entirely the right thing either.  What do you 
> think about it?

I don't think we need another Clang built-in. __builtin_astype was added 
specifically for OpenCL needs (see rev. 132612).
Do you know better way to map astype operators (there are a lot of them 
as_#) to single Clang built-in?


https://reviews.llvm.org/D28136



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

[PATCH] D28136: [OpenCL] Implement as_type operator as alias of __builtin_astype.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Headers/opencl-c.h:6588
-char __ovld __cnfn as_char(char);
-char __ovld __cnfn as_char(uchar);
-

Anastasia wrote:
> Why do we have some of the overloads omitted? Would this cause any extra 
> conversions? uchar -> char in this case?
It's specific to how builtin_astype works. It drops first argument type and 
only cares about matching first argument size with the data type size provided 
as a second argument. So basically with single line we get all possible and 
impossible overloads of as_char(*).
This define will pass any variable type char,uchar to builtin_astype.


https://reviews.llvm.org/D28136



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

[PATCH] D30643: [OpenCL] Extended diagnostics for atomic initialization

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8263
+def err_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global 
address space">;
 def err_atomic_init_constant : Error<

bader wrote:
> Anastasia wrote:
> > echuraev wrote:
> > > Anastasia wrote:
> > > > Could we combine this error diag with the one below? I guess they are 
> > > > semantically very similar apart from one is about initialization and 
> > > > another is about assignment?
> > > I'm not sure that it is a good idea to combine these errors. For example, 
> > > if developer had declared a variable non-constant and not in global 
> > > address space he would have got the same message for both errors. And it 
> > > can be difficult to determine what the exact problem is. He can fix one 
> > > of the problems but he will still get the same error.
> > Well, I don't actually see that we check for constant anywhere so it's also 
> > OK if you want to drop this bit. Although I think the original intension of 
> > this message as I understood was to provide the most complete hint.
> > 
> > My concern is that these two errors seem to be reporting nearly the same 
> > issue and ideally we would like to keep diagnostic list as small as 
> > possible. This also makes the file more concise and messages more 
> > consistent.
> I suggest adding a test case with non-constant initialization case to 
> validate that existing checks cover this case for atomic types already.
> If so, we can adjust existing diagnostic message to cover both cases: 
> initialization and assignment expression.
I don't think it's quite true.
There are two requirements here that must be met the same time. Atomic 
variables *declared in the global address space* can be initialized only with 
"compile time constant'.
If understand the spec correctly this code is also valid:

  kernel void foo() {
static global atomic_int a = 42; // although it's not clear if we must use 
ATOMIC_VAR_INIT here.
...
  }



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8267
   "variable in constant address space must be initialized">;
-def err_atomic_init_constant : Error<
-  "atomic variable can only be assigned to a compile time constant"
-  " in the declaration statement in the program scope">;
+// Atomics
+def err_atomic_init: Error<

I suggest removing this comment.
If you are going to add other diagnostic messages specific to OpenCL atomics, 
then separate them from this list of unordered diagnostics similar to pipe 
built-in functions below.


https://reviews.llvm.org/D30643



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

[PATCH] D30816: [OpenCL] Added implicit conversion rank for overloading functions with vector data type in OpenCL

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/SemaOpenCL/overload_addrspace_resolution.cl:1
-// RUN: %clang_cc1 -cl-std=CL2.0 -emit-llvm -o - -triple 
x86_64-unknown-unknown %s | FileCheck %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -emit-llvm -o - -triple spir-unknown-unknown 
%s | FileCheck %s
 

Egor, I think you forgot to move the test to CodeGenOpenCL directory.


https://reviews.llvm.org/D30816



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

[PATCH] D30643: [OpenCL] Extended diagnostics for atomic initialization

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8263
+def err_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global 
address space">;
 def err_atomic_init_constant : Error<

Anastasia wrote:
> bader wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > echuraev wrote:
> > > > > Anastasia wrote:
> > > > > > Could we combine this error diag with the one below? I guess they 
> > > > > > are semantically very similar apart from one is about 
> > > > > > initialization and another is about assignment?
> > > > > I'm not sure that it is a good idea to combine these errors. For 
> > > > > example, if developer had declared a variable non-constant and not in 
> > > > > global address space he would have got the same message for both 
> > > > > errors. And it can be difficult to determine what the exact problem 
> > > > > is. He can fix one of the problems but he will still get the same 
> > > > > error.
> > > > Well, I don't actually see that we check for constant anywhere so it's 
> > > > also OK if you want to drop this bit. Although I think the original 
> > > > intension of this message as I understood was to provide the most 
> > > > complete hint.
> > > > 
> > > > My concern is that these two errors seem to be reporting nearly the 
> > > > same issue and ideally we would like to keep diagnostic list as small 
> > > > as possible. This also makes the file more concise and messages more 
> > > > consistent.
> > > I suggest adding a test case with non-constant initialization case to 
> > > validate that existing checks cover this case for atomic types already.
> > > If so, we can adjust existing diagnostic message to cover both cases: 
> > > initialization and assignment expression.
> > I don't think it's quite true.
> > There are two requirements here that must be met the same time. Atomic 
> > variables *declared in the global address space* can be initialized only 
> > with "compile time constant'.
> > If understand the spec correctly this code is also valid:
> > 
> >   kernel void foo() {
> > static global atomic_int a = 42; // although it's not clear if we must 
> > use ATOMIC_VAR_INIT here.
> > ...
> >   }
> Precisely, but I think checking for compile time constant should be inherited 
> from the general C implementation? I don't think we do anything extra for it. 
>  Regarding the macro I am not sure we can suitably diagnose it anyways...
> Precisely, but I think checking for compile time constant should be inherited 
> from the general C implementation?

Agree. I suggested checking this above by extending OpenCL tests, but this can 
be done separately.


https://reviews.llvm.org/D30643



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

[PATCH] D30643: [OpenCL] Extended diagnostics for atomic initialization

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8263
+def err_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global 
address space">;
 def err_atomic_init_constant : Error<

Anastasia wrote:
> echuraev wrote:
> > Anastasia wrote:
> > > Could we combine this error diag with the one below? I guess they are 
> > > semantically very similar apart from one is about initialization and 
> > > another is about assignment?
> > I'm not sure that it is a good idea to combine these errors. For example, 
> > if developer had declared a variable non-constant and not in global address 
> > space he would have got the same message for both errors. And it can be 
> > difficult to determine what the exact problem is. He can fix one of the 
> > problems but he will still get the same error.
> Well, I don't actually see that we check for constant anywhere so it's also 
> OK if you want to drop this bit. Although I think the original intension of 
> this message as I understood was to provide the most complete hint.
> 
> My concern is that these two errors seem to be reporting nearly the same 
> issue and ideally we would like to keep diagnostic list as small as possible. 
> This also makes the file more concise and messages more consistent.
I suggest adding a test case with non-constant initialization case to validate 
that existing checks cover this case for atomic types already.
If so, we can adjust existing diagnostic message to cover both cases: 
initialization and assignment expression.


https://reviews.llvm.org/D30643



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

[PATCH] D28136: [OpenCL] Implement as_type operator as alias of __builtin_astype.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

> Why do you think this is a bug? It seems to follow standard behavior in C to 
> promote char to int if required. Just like if you would have a C code:
> 
>   int as_int(int i);
>   void foo() {
>   char src = 1;
>   int dst = as_int(src);
>   } 
>
> 
> This code would complie and the same exactly IR would be generated.

as_type is defined to be used for bit re-interpretation. (see 6.2.4.2 
Reinterpreting Types Using as_type() and as_typen()). In this sense, it's 
exactly matches __bultin_astype built-in function.

Here are a few relevant OpenCL C language specification quotes from 6.2.4 
section:

> All data types described in tables 6.1 and 6.2 (except bool, half and void) 
> may be also reinterpreted as another data type of **the same size** using the 
> as_type() operator for scalar data types and the as_typen() operator for 
> vector data types.



> The usual type promotion for function arguments shall not be performed.



> It is an error to use as_type() or as_typen() operator to reinterpret data to 
> a type of a different number of bytes.

So, aliasing as_type to __builtin_astype provides these checks, whereas we 
can't do it for overloadable as_type function declarations.

I also would like to address your original concerns:

> The main issue is after preprocessing the header the original function name 
> is no longer available in diagnostics reported.

Actually diagnostics is able to provide a hint to exact code location in the 
header file where as_ is defined as macro, so user gets correct name of 
the operator in diagnostics as far as I know.

> The spec defines as_type as a builtin function and not a macro.

To be precise, spec defines as_type as an operator. So, the best way to 
implement it would be to add a language support of such operator, but AFAIK 
aliasing to __bulitin_astype via macro is sufficient enough for OpenCL use 
cases.

> Additionally your patch would allow as_type to be used with extra type (not 
> only those defined in spec).

Not sure I get this. We defined limited set of as_* functions - only for types 
from tables 6.1 and 6.2 as specified by specification, so if OpenCL developer 
will try to call as_(type2), which is not defined in the header, 
compiler will report en error about calling undeclared function.

> Also I don't see the problem to implement as_type with just simply calling a 
> builtin. It should be inlined later anyways.

Yes, but this solution will not give us error checking as pre-processor 
solution.

Does it make sense?


https://reviews.llvm.org/D28136



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

[PATCH] D36044: [OpenCL] -cl-ext option can overwrite OpenCL features imported from a module

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/SemaOpenCL/extensions-import.cl:14
+
+#ifdef FP64
+// expected-no-diagnostics

You can use `#ifdef cl_khr_fp64` to check if extension is supported.


https://reviews.llvm.org/D36044



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

[PATCH] D36259: [OpenCL] Remove extra select functions from opencl-c.h

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

Out of curiosity: how did you detect this?
Can we use the same approach for writing tests?


https://reviews.llvm.org/D36259



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

[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Headers/opencl-c.h:16020
+// The macro CLK_NULL_RESERVE_ID refers to an invalid reservation ID.
+#define CLK_NULL_RESERVE_ID (__builtin_astype((void *)0, reserve_id_t))
 bool __ovld is_valid_reserve_id(reserve_id_t reserve_id);

b-sumner wrote:
> yaxunl wrote:
> > bader wrote:
> > > yaxunl wrote:
> > > > bader wrote:
> > > > > yaxunl wrote:
> > > > > > bader wrote:
> > > > > > > yaxunl wrote:
> > > > > > > > yaxunl wrote:
> > > > > > > > > bader wrote:
> > > > > > > > > > yaxunl wrote:
> > > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > > echuraev wrote:
> > > > > > > > > > > > > yaxunl wrote:
> > > > > > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > > > > > yaxunl wrote:
> > > > > > > > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > > > > > > > Looks good from my side.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > @yaxunl , since you originally committed 
> > > > > > > > > > > > > > > > > this. Could you please verify that changing 
> > > > > > > > > > > > > > > > > from `SIZE_MAX` to `0` would be fine.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > Btw, we have a similar definition for 
> > > > > > > > > > > > > > > > > `CLK_NULL_EVENT`.
> > > > > > > > > > > > > > > > `__PIPE_RESERVE_ID_VALID_BIT` is implementation 
> > > > > > > > > > > > > > > > detail and not part of the spec. I would 
> > > > > > > > > > > > > > > > suggest to remove it from this header file.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > The spec only requires CLK_NULL_RESERVE_ID to 
> > > > > > > > > > > > > > > > be defined but does not define its value. 
> > > > > > > > > > > > > > > > Naturally a valid id starts from 0 and 
> > > > > > > > > > > > > > > > increases. I don't see significant advantage to 
> > > > > > > > > > > > > > > > change CLK_NULL_RESERVE_ID from __SIZE_MAX to 0.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Is there any reason that this change is needed?
> > > > > > > > > > > > > > > I don't see issues to commit things outside of 
> > > > > > > > > > > > > > > spec as soon as they prefixed properly with "__". 
> > > > > > > > > > > > > > >  But I agree it would be nice to see if it's any 
> > > > > > > > > > > > > > > useful and what the motivation is for having 
> > > > > > > > > > > > > > > different implementation.
> > > > > > > > > > > > > > For `__PIPE_RESERVE_ID_VALID_BIT`, it assumes that 
> > > > > > > > > > > > > > the implementation uses one specific bit of a 
> > > > > > > > > > > > > > reserve id to indicate that the reserve id is 
> > > > > > > > > > > > > > valid. Not all implementations assume that. 
> > > > > > > > > > > > > > Actually I am curious why that is needed too.
> > > > > > > > > > > > > About `CLK_NULL_RESERVE_ID`: we check that reserve id 
> > > > > > > > > > > > > is valid if significant bit equal to one. 
> > > > > > > > > > > > > `CLK_NULL_RESERVE_ID refers to an invalid 
> > > > > > > > > > > > > reservation, so if `CLK_NULL_RESERVE_ID equal to 0, 
> > > > > > > > > > > > > we can be sure that significant bit doesn't equal to 
> > > > > > > > > > > > > 1 and it is invalid reserve id. Also it is more 
> > > > > > > > > > > > > obviously if CLK_**NULL**_RESERVE_ID is equal to 0.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > What about `__PIPE_RESERVE_ID_VALID_BIT`: As I 
> > > > > > > > > > > > > understand previous implementation also assumes that 
> > > > > > > > > > > > > one specific bit was of a reverse id was used to 
> > > > > > > > > > > > > indicate that the reserve id is valid. So, we just 
> > > > > > > > > > > > > increased reserve id size by one bit on 32-bit 
> > > > > > > > > > > > > platforms and by 33 bits on 64-bit platforms. 
> > > > > > > > > > > > It is more logical to me that `CLK_NULL_RESERVE_ID` is 
> > > > > > > > > > > > 0, but spec doesn't define it of course.
> > > > > > > > > > > In our implementation, valid reserve id starts at 0 and 
> > > > > > > > > > > increasing linearly until `__SIZE_MAX-1`. This change 
> > > > > > > > > > > will break our implementation.
> > > > > > > > > > > 
> > > > > > > > > > > However, we can modify our implementation to adopt this 
> > > > > > > > > > > change since it brings about benefits overall.
> > > > > > > > > > Ideally it would be great to have unified implementation, 
> > > > > > > > > > but we can define device specific value for 
> > > > > > > > > > CLK_NULL_RESERVE_ID by using ifdef directive.
> > > > > > > > > How about
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > __attribute__((const)) size_t __clk_null_reserve_id();
> > > > > > > > > #define CLK_NULL_RESERVE_ID __clk_null_reserve_id()
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > I think the spec does not require it to be compile time 
> > > > > > > > > constant. Then each library can implement its own 
> > > > > > > > > 

[PATCH] D36676: Remove -finclude-default-header in OpenCL atomic tests

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks.


https://reviews.llvm.org/D36676



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

[PATCH] D36327: [OpenCL] Allow targets emit optimized pipe functions for power of 2 type sizes

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D36327#840616, @yaxunl wrote:

> In https://reviews.llvm.org/D36327#839809, @rjmccall wrote:
>
> > Could you just implement this in SimplifyLibCalls?  I assume there's some 
> > way to fill in TargetLibraryInfo appropriately for a platform.  Is that too 
> > late for your linking requirements?
>
>
> Both the optimized and generic versions of __read_pipe function contains call 
> of other library functions and are complicate enough not to be generated 
> programmatically. amdgpu target does not have the capability to link in 
> library code after LLVM codegen. The linking has to be done before 
> SimplifyLibCalls.


If I understand correctly, SimplifyLibCalls is LLVM IR transformation, so it 
works before linking and LLVM codegen (e.g. InstCombine passes run this 
transformation). This pass is doing something similar to what you are trying to 
achieve for __read_pipe builti-ins: pow(2.0, x) -> llvm.exp2(x).


https://reviews.llvm.org/D36327



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

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


https://reviews.llvm.org/D35082



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

[PATCH] D33945: [OpenCL] Add support for missing sub_group functions.

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

Thanks!
Overall the patch looks good, but I would suggest splitting it into three 
commits (as they seems to be independent):

1. [OpenCL] Check that cl_khr_subgroups pragma is enabled if respective 
extension is used.
2. [OpenCL] Add support for missing sub_group functions.
3. [OpenCL] Fix return type for reserve pipe built-ins.

Please, add a regression test for the part #3.

You might also review this patch with @Anastasia (OpenCL code owner).




Comment at: Sema/SemaChecking.cpp:685-689
+  // Since return type of reserve_read/write_pipe built-in function is
+  // reserve_id_t, which is not defined in the builtin def file , we used int
+  // as return type and need to override the return type of these functions.
+  Call->setType(S.Context.OCLReserveIDTy);
+

This change is not covered with regression tests.


https://reviews.llvm.org/D33945



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

[PATCH] D34871: [OpenCL] Add function name to extension diagnostic

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.
Just one minor comment.
Thanks.




Comment at: clang/Sema/Sema.h:8435
   /// \return true if type is disabled.
-  bool checkOpenCLDisabledDecl(const Decl , const Expr );
+  bool checkOpenCLDisabledDecl(const FunctionDecl , const Expr );
 

I think it's better to use NamedDecl to allow this function be used for 
declarations other than FunctionDecl.


https://reviews.llvm.org/D34871



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

[PATCH] D34948: [OpenCL] Generalise err_opencl_enqueue_kernel_expected_type to be used with other builtins

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.
Do you have another built-in in mind which can use this diagnostic message?
If so, it would make sense to re-use it in the same patch.


https://reviews.llvm.org/D34948



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

[PATCH] D33681: [OpenCL] Allow function declaration with empty argument list.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@chapuni, thanks for taking care of this. I'll take a look.


https://reviews.llvm.org/D33681



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

[PATCH] D34948: [OpenCL] Generalise err_opencl_enqueue_kernel_expected_type to be used with other builtins

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

>> Do you have another built-in in mind which can use this diagnostic message?
>>  If so, it would make sense to re-use it in the same patch.
> 
> This is split off from https://reviews.llvm.org/D33945, which I will be 
> rebasing/re-uploading once this patch is committed.

I see. Thanks for clarification.
No other comments from my side.


https://reviews.llvm.org/D34948



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

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Note: `get_sampler_initializer` from my test case returns integer, not a 
sampler, but having function is not relevant to the problem. 
Here is a bit simplified test case without function calls that still reproduces 
the problem:

  kernel void foo(int sampler_init_value) {
const sampler_t const_smp_func_init = sampler_init_value;
  }

The problem is in the function that handles sampler initialization with integer.
There should no problems with the case you provided as it doesn't require 
additional function call injection.


https://reviews.llvm.org/D34342



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

[PATCH] D36327: [OpenCL] Allow targets emit optimized pipe functions for power of 2 type sizes

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@rsmith do you have an opinion on what would be the right place for the kind of 
proposed optimization?
It looks like it can be implemented as target independent optimization, acting 
only for target with specified properties - in this case target must provide 
required built-in functions.


https://reviews.llvm.org/D36327



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

[PATCH] D36327: [OpenCL] Allow targets emit optimized pipe functions for power of 2 type sizes

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Hi Sam,

What do you think about implementing this optimization in target specific 
optimization pass? Since size/alignment is saved as function parameter in LLVM 
IR, the optimization can be done in target specific components w/o adding 
additional conditions to generic library.

Thanks,
Alexey


https://reviews.llvm.org/D36327



___
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

[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] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Headers/opencl-c.h:16020
+// The macro CLK_NULL_RESERVE_ID refers to an invalid reservation ID.
+#define CLK_NULL_RESERVE_ID (__builtin_astype((void *)0, reserve_id_t))
 bool __ovld is_valid_reserve_id(reserve_id_t reserve_id);

yaxunl wrote:
> Anastasia wrote:
> > echuraev wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > yaxunl wrote:
> > > > > > Anastasia wrote:
> > > > > > > Looks good from my side.
> > > > > > > 
> > > > > > > @yaxunl , since you originally committed this. Could you please 
> > > > > > > verify that changing from `SIZE_MAX` to `0` would be fine.
> > > > > > > 
> > > > > > > Btw, we have a similar definition for `CLK_NULL_EVENT`.
> > > > > > `__PIPE_RESERVE_ID_VALID_BIT` is implementation detail and not part 
> > > > > > of the spec. I would suggest to remove it from this header file.
> > > > > > 
> > > > > > The spec only requires CLK_NULL_RESERVE_ID to be defined but does 
> > > > > > not define its value. Naturally a valid id starts from 0 and 
> > > > > > increases. I don't see significant advantage to change 
> > > > > > CLK_NULL_RESERVE_ID from __SIZE_MAX to 0.
> > > > > > 
> > > > > > Is there any reason that this change is needed?
> > > > > I don't see issues to commit things outside of spec as soon as they 
> > > > > prefixed properly with "__".  But I agree it would be nice to see if 
> > > > > it's any useful and what the motivation is for having different 
> > > > > implementation.
> > > > For `__PIPE_RESERVE_ID_VALID_BIT`, it assumes that the implementation 
> > > > uses one specific bit of a reserve id to indicate that the reserve id 
> > > > is valid. Not all implementations assume that. Actually I am curious 
> > > > why that is needed too.
> > > About `CLK_NULL_RESERVE_ID`: we check that reserve id is valid if 
> > > significant bit equal to one. `CLK_NULL_RESERVE_ID refers to an invalid 
> > > reservation, so if `CLK_NULL_RESERVE_ID equal to 0, we can be sure that 
> > > significant bit doesn't equal to 1 and it is invalid reserve id. Also it 
> > > is more obviously if CLK_**NULL**_RESERVE_ID is equal to 0.
> > > 
> > > What about `__PIPE_RESERVE_ID_VALID_BIT`: As I understand previous 
> > > implementation also assumes that one specific bit was of a reverse id was 
> > > used to indicate that the reserve id is valid. So, we just increased 
> > > reserve id size by one bit on 32-bit platforms and by 33 bits on 64-bit 
> > > platforms. 
> > It is more logical to me that `CLK_NULL_RESERVE_ID` is 0, but spec doesn't 
> > define it of course.
> In our implementation, valid reserve id starts at 0 and increasing linearly 
> until `__SIZE_MAX-1`. This change will break our implementation.
> 
> However, we can modify our implementation to adopt this change since it 
> brings about benefits overall.
Ideally it would be great to have unified implementation, but we can define 
device specific value for CLK_NULL_RESERVE_ID by using ifdef directive.


https://reviews.llvm.org/D32896



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

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/CodeGenOpenCL/sampler.cl:62
+
+  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 35)

yaxunl wrote:
> what if address of const_smp is taken and assigned to a pointer to sampler_t 
> ? Do we have diagnosis in place?
AFAIK, we have diagnostics for both:
- declaration of a pointer to sampler
- taking address of sampler variable


https://reviews.llvm.org/D34342



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

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 103421.
bader added a comment.
This revision is now accepted and ready to land.

Added test case reproducing the issue described in the description.
Removed test cases from test/SemaOpenCL/sampler_t.cl covered by 
test/CodeGenOpenCL/sampler.cl.

While I was moving one test case from test/SemaOpenCL/sampler_t.cl, I found 
another bug in CodeGen library that crashes the compilation if sampler is 
initialized with non-constant expression.

Here is a short reproducer:

  int get_sampler_initializer(void);
  kernel void foo() {
const sampler_t const_smp_func_init = get_sampler_initializer();
  }

The problem is that clang does not discard this code as invalid, but CodeGen 
library expects sampler initializer to be a constant expression:

  llvm::Value *
  CodeGenModule::createOpenCLIntToSamplerConversion(const Expr *E,
CodeGenFunction ) {
llvm::Constant *C = EmitConstantExpr(E, E->getType(), ); // for the 
reproducer expression here is CallExpr.
auto SamplerT = getOpenCLRuntime().getSamplerType();
auto FTy = llvm::FunctionType::get(SamplerT, {C->getType()}, false);
return CGF.Builder.CreateCall(CreateRuntimeFunction(FTy,
  "__translate_sampler_initializer"),
  {C});
  }

There are two ways to handle this issue:

1. Implement diagnostics allowing only compile time constant initializers.
2. Add non-constant sampler initializer support to CodeGen library.

OpenCL specification examples give me impression that samplers declared inside 
OpenCL programs must be known at compile time.
On the other hand OpenCL allows samplers passed via kernel parameters to be 
unknown at compile time.

Thoughts?


https://reviews.llvm.org/D34342

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenOpenCL/sampler.cl
  test/SemaOpenCL/sampler_t.cl


Index: test/SemaOpenCL/sampler_t.cl
===
--- test/SemaOpenCL/sampler_t.cl
+++ test/SemaOpenCL/sampler_t.cl
@@ -46,36 +46,11 @@
 
 void kernel ker(sampler_t argsmp) {
   local sampler_t smp; // expected-error{{sampler type cannot be used with the 
__local and __global address space qualifiers}}
-  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
-  const sampler_t const_smp2;
-  const sampler_t const_smp3 = const_smp;
-  const sampler_t const_smp4 = f();
   const sampler_t const_smp5 = 1.0f; // expected-error{{initializing 'const 
sampler_t' with an expression of incompatible type 'float'}}
   const sampler_t const_smp6 = 0x1LL; // expected-error{{sampler_t 
initialization requires 32-bit integer, not 'long long'}}
 
-  foo(glb_smp);
-  foo(glb_smp2);
-  foo(glb_smp3);
-  foo(glb_smp4);
-  foo(glb_smp5);
-  foo(glb_smp6);
-  foo(glb_smp7);
-  foo(glb_smp8);
-  foo(glb_smp9);
-  foo(smp);
-  foo(sampler_str.smp);
-  foo(const_smp);
-  foo(const_smp2);
-  foo(const_smp3);
-  foo(const_smp4);
-  foo(const_smp5);
-  foo(const_smp6);
-  foo(argsmp);
-  foo(5);
   foo(5.0f); // expected-error {{passing 'float' to parameter of incompatible 
type 'sampler_t'}}
-  sampler_t sa[] = {argsmp, const_smp}; // expected-error {{array of 
'sampler_t' type is invalid in OpenCL}}
-  foo(sa[0]);
-  foo(bad());
+  sampler_t sa[] = {argsmp, glb_smp}; // expected-error {{array of 'sampler_t' 
type is invalid in OpenCL}}
 }
 
 void bad(sampler_t*); // expected-error{{pointer to type 'sampler_t' is 
invalid in OpenCL}}
Index: test/CodeGenOpenCL/sampler.cl
===
--- test/CodeGenOpenCL/sampler.cl
+++ test/CodeGenOpenCL/sampler.cl
@@ -20,6 +20,8 @@
 constant sampler_t glb_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
 // CHECK-NOT: glb_smp
 
+int get_sampler_initializer(void);
+
 void fnc4smp(sampler_t s) {}
 // CHECK: define spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* %
 
@@ -58,4 +60,20 @@
   fnc4smp(5);
   // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 5)
   // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
+
+  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  fnc4smp(const_smp);
+   // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 35)
+  // CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], 
%opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]]
+  fnc4smp(const_smp);
+  // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, 
%opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]]
+  // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
+
+  constant sampler_t constant_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

I think I found the way to reproduce the issue. There are two pre-requisites: 
sampler must be declared in the constant address space and clang must be build 
in Debug mode.
I'll fix the test and add the test cases from test/SemaOpenCL/sampler_t.cl as 
mentioned earlier.


https://reviews.llvm.org/D34342



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

[PATCH] D33681: [OpenCL] Allow function declaration with empty argument list.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Ping.

Although this patch is already accepted, I'd like to confirm that I can commit 
the latest update with changes in test/SemaOpenCL/invalid-pipes-cl2.0.cl.

Thanks.


https://reviews.llvm.org/D33681



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

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

Wow...
Nice catch.
For some reason I can't reproduce the problem neither.
I will drop source code change, but I'd like to modify the test anyway.
https://reviews.llvm.org/rL277024 added test/CodeGenOpenCL/sampler.cl and 
modified test/SemaOpenCL/sampler_t.cl.
I want to move the parts of the test/SemaOpenCL/sampler_t.cl, which are 
supposed to pass semantic analysis to test/CodeGenOpenCL/sampler.cl and 
validate the LLVM IR after code generation.
Are you OK with this?


https://reviews.llvm.org/D34342



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

[PATCH] D34235: [OpenCL] Fix OpenCL and SPIR version metadata generation.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/CodeGenOpenCL/spir_version.cl:13
 kernel void foo() {}
+kernel void bar() {}
 

Anastasia wrote:
> Would the original code produce duplicate version metadata here or is it just 
> for overloaded functions? Would it make sense to add `CHECK-NOT` to make sure 
> they are not generated twice?
The original code will duplicate the metadata here. Something like:
```
!opencl.ocl.version = !{!0, !0}
```
One per global value - function in case.

Existing check is already good enough as it checks exactly for one metadata:
```
// CHECK-SPIR-CL10-DAG: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
```
It was passing with buggy code, since test contains exactly one function (i.e. 
global value). Now we have two global values and fix is required to pass 
existing check. 


https://reviews.llvm.org/D34235



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

[PATCH] D33353: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

This issue probably can be fixed by setting SPIR target architecture or just 
enabling cl_khr_fp64 extension.
Unfortunately, I can't check it today.
Could you revert Egor's commit, please?


https://reviews.llvm.org/D33353



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

[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Headers/opencl-c.h:16020
+// The macro CLK_NULL_RESERVE_ID refers to an invalid reservation ID.
+#define CLK_NULL_RESERVE_ID (__builtin_astype((void *)0, reserve_id_t))
 bool __ovld is_valid_reserve_id(reserve_id_t reserve_id);

yaxunl wrote:
> yaxunl wrote:
> > bader wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > echuraev wrote:
> > > > > > yaxunl wrote:
> > > > > > > Anastasia wrote:
> > > > > > > > yaxunl wrote:
> > > > > > > > > Anastasia wrote:
> > > > > > > > > > Looks good from my side.
> > > > > > > > > > 
> > > > > > > > > > @yaxunl , since you originally committed this. Could you 
> > > > > > > > > > please verify that changing from `SIZE_MAX` to `0` would be 
> > > > > > > > > > fine.
> > > > > > > > > > 
> > > > > > > > > > Btw, we have a similar definition for `CLK_NULL_EVENT`.
> > > > > > > > > `__PIPE_RESERVE_ID_VALID_BIT` is implementation detail and 
> > > > > > > > > not part of the spec. I would suggest to remove it from this 
> > > > > > > > > header file.
> > > > > > > > > 
> > > > > > > > > The spec only requires CLK_NULL_RESERVE_ID to be defined but 
> > > > > > > > > does not define its value. Naturally a valid id starts from 0 
> > > > > > > > > and increases. I don't see significant advantage to change 
> > > > > > > > > CLK_NULL_RESERVE_ID from __SIZE_MAX to 0.
> > > > > > > > > 
> > > > > > > > > Is there any reason that this change is needed?
> > > > > > > > I don't see issues to commit things outside of spec as soon as 
> > > > > > > > they prefixed properly with "__".  But I agree it would be nice 
> > > > > > > > to see if it's any useful and what the motivation is for having 
> > > > > > > > different implementation.
> > > > > > > For `__PIPE_RESERVE_ID_VALID_BIT`, it assumes that the 
> > > > > > > implementation uses one specific bit of a reserve id to indicate 
> > > > > > > that the reserve id is valid. Not all implementations assume 
> > > > > > > that. Actually I am curious why that is needed too.
> > > > > > About `CLK_NULL_RESERVE_ID`: we check that reserve id is valid if 
> > > > > > significant bit equal to one. `CLK_NULL_RESERVE_ID refers to an 
> > > > > > invalid reservation, so if `CLK_NULL_RESERVE_ID equal to 0, we can 
> > > > > > be sure that significant bit doesn't equal to 1 and it is invalid 
> > > > > > reserve id. Also it is more obviously if CLK_**NULL**_RESERVE_ID is 
> > > > > > equal to 0.
> > > > > > 
> > > > > > What about `__PIPE_RESERVE_ID_VALID_BIT`: As I understand previous 
> > > > > > implementation also assumes that one specific bit was of a reverse 
> > > > > > id was used to indicate that the reserve id is valid. So, we just 
> > > > > > increased reserve id size by one bit on 32-bit platforms and by 33 
> > > > > > bits on 64-bit platforms. 
> > > > > It is more logical to me that `CLK_NULL_RESERVE_ID` is 0, but spec 
> > > > > doesn't define it of course.
> > > > In our implementation, valid reserve id starts at 0 and increasing 
> > > > linearly until `__SIZE_MAX-1`. This change will break our 
> > > > implementation.
> > > > 
> > > > However, we can modify our implementation to adopt this change since it 
> > > > brings about benefits overall.
> > > Ideally it would be great to have unified implementation, but we can 
> > > define device specific value for CLK_NULL_RESERVE_ID by using ifdef 
> > > directive.
> > How about
> > 
> > ```
> > __attribute__((const)) size_t __clk_null_reserve_id();
> > #define CLK_NULL_RESERVE_ID __clk_null_reserve_id()
> > 
> > ```
> > I think the spec does not require it to be compile time constant. Then each 
> > library can implement its own __clk_null_reserve_id() whereas the IR is 
> > target independent.
> Or we only do this for SPIR and define it as target specific value for other 
> targets.
Defining CLK_NULL_RESERVE_ID as a function call should also work, but IMHO 
compile time constant is preferable option.
I don't think making it compile time constant for SPIR only makes sense to me - 
in this case we can use constant for all targets.

How about following approach: use 0 by default and allow other targets 
re-define CLK_NULL_RESERVE_ID value.

```
#ifndef CLK_NULL_RESERVE_ID
  #define CLK_NULL_RESERVE_ID 0
#endif // CLK_NULL_RESERVE_ID 
```

If CLK_NULL_RESERVE_ID defined via -D command line option or included before 
OpenCL C header file (via -include option), the defined value will be used, 
otherwise 0.

Will it work for you?


https://reviews.llvm.org/D32896



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

[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Headers/opencl-c.h:16020
+// The macro CLK_NULL_RESERVE_ID refers to an invalid reservation ID.
+#define CLK_NULL_RESERVE_ID (__builtin_astype((void *)0, reserve_id_t))
 bool __ovld is_valid_reserve_id(reserve_id_t reserve_id);

yaxunl wrote:
> bader wrote:
> > yaxunl wrote:
> > > yaxunl wrote:
> > > > bader wrote:
> > > > > yaxunl wrote:
> > > > > > Anastasia wrote:
> > > > > > > echuraev wrote:
> > > > > > > > yaxunl wrote:
> > > > > > > > > Anastasia wrote:
> > > > > > > > > > yaxunl wrote:
> > > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > > Looks good from my side.
> > > > > > > > > > > > 
> > > > > > > > > > > > @yaxunl , since you originally committed this. Could 
> > > > > > > > > > > > you please verify that changing from `SIZE_MAX` to `0` 
> > > > > > > > > > > > would be fine.
> > > > > > > > > > > > 
> > > > > > > > > > > > Btw, we have a similar definition for `CLK_NULL_EVENT`.
> > > > > > > > > > > `__PIPE_RESERVE_ID_VALID_BIT` is implementation detail 
> > > > > > > > > > > and not part of the spec. I would suggest to remove it 
> > > > > > > > > > > from this header file.
> > > > > > > > > > > 
> > > > > > > > > > > The spec only requires CLK_NULL_RESERVE_ID to be defined 
> > > > > > > > > > > but does not define its value. Naturally a valid id 
> > > > > > > > > > > starts from 0 and increases. I don't see significant 
> > > > > > > > > > > advantage to change CLK_NULL_RESERVE_ID from __SIZE_MAX 
> > > > > > > > > > > to 0.
> > > > > > > > > > > 
> > > > > > > > > > > Is there any reason that this change is needed?
> > > > > > > > > > I don't see issues to commit things outside of spec as soon 
> > > > > > > > > > as they prefixed properly with "__".  But I agree it would 
> > > > > > > > > > be nice to see if it's any useful and what the motivation 
> > > > > > > > > > is for having different implementation.
> > > > > > > > > For `__PIPE_RESERVE_ID_VALID_BIT`, it assumes that the 
> > > > > > > > > implementation uses one specific bit of a reserve id to 
> > > > > > > > > indicate that the reserve id is valid. Not all 
> > > > > > > > > implementations assume that. Actually I am curious why that 
> > > > > > > > > is needed too.
> > > > > > > > About `CLK_NULL_RESERVE_ID`: we check that reserve id is valid 
> > > > > > > > if significant bit equal to one. `CLK_NULL_RESERVE_ID refers to 
> > > > > > > > an invalid reservation, so if `CLK_NULL_RESERVE_ID equal to 0, 
> > > > > > > > we can be sure that significant bit doesn't equal to 1 and it 
> > > > > > > > is invalid reserve id. Also it is more obviously if 
> > > > > > > > CLK_**NULL**_RESERVE_ID is equal to 0.
> > > > > > > > 
> > > > > > > > What about `__PIPE_RESERVE_ID_VALID_BIT`: As I understand 
> > > > > > > > previous implementation also assumes that one specific bit was 
> > > > > > > > of a reverse id was used to indicate that the reserve id is 
> > > > > > > > valid. So, we just increased reserve id size by one bit on 
> > > > > > > > 32-bit platforms and by 33 bits on 64-bit platforms. 
> > > > > > > It is more logical to me that `CLK_NULL_RESERVE_ID` is 0, but 
> > > > > > > spec doesn't define it of course.
> > > > > > In our implementation, valid reserve id starts at 0 and increasing 
> > > > > > linearly until `__SIZE_MAX-1`. This change will break our 
> > > > > > implementation.
> > > > > > 
> > > > > > However, we can modify our implementation to adopt this change 
> > > > > > since it brings about benefits overall.
> > > > > Ideally it would be great to have unified implementation, but we can 
> > > > > define device specific value for CLK_NULL_RESERVE_ID by using ifdef 
> > > > > directive.
> > > > How about
> > > > 
> > > > ```
> > > > __attribute__((const)) size_t __clk_null_reserve_id();
> > > > #define CLK_NULL_RESERVE_ID __clk_null_reserve_id()
> > > > 
> > > > ```
> > > > I think the spec does not require it to be compile time constant. Then 
> > > > each library can implement its own __clk_null_reserve_id() whereas the 
> > > > IR is target independent.
> > > Or we only do this for SPIR and define it as target specific value for 
> > > other targets.
> > Defining CLK_NULL_RESERVE_ID as a function call should also work, but IMHO 
> > compile time constant is preferable option.
> > I don't think making it compile time constant for SPIR only makes sense to 
> > me - in this case we can use constant for all targets.
> > 
> > How about following approach: use 0 by default and allow other targets 
> > re-define CLK_NULL_RESERVE_ID value.
> > 
> > ```
> > #ifndef CLK_NULL_RESERVE_ID
> >   #define CLK_NULL_RESERVE_ID 0
> > #endif // CLK_NULL_RESERVE_ID 
> > ```
> > 
> > If CLK_NULL_RESERVE_ID defined via -D command line option or included 
> > before OpenCL C header file (via -include option), the defined value will 
> > be used, otherwise 0.
> > 
> > Will it work for you?
> No. That makes us unable to consume SPIR since 

[PATCH] D33821: [OpenCL] Harden function pointer diagnostics.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.

Improve OpenCL type checking by rejecting function pointer types.


https://reviews.llvm.org/D33821

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaType.cpp
  test/SemaOpenCL/func.cl


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -4,8 +4,15 @@
 void vararg_f(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
 typedef void (*vararg_fptr_t)(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
+// expected-error@-1{{pointers to 
functions are not allowed}}
 int printf(__constant const char *st, ...); // expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 
+// Struct type with function pointer field
+typedef struct s
+{
+   void (*f)(struct s *self, int *i);   // expected-error{{pointers to 
functions are not allowed}}
+} s_t;
+
 //Function pointer
 void foo(void*);
 
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -1881,6 +1881,11 @@
 return QualType();
   }
 
+  if (T->isFunctionType() && getLangOpts().OpenCL) {
+Diag(Loc, diag::err_opencl_function_pointer);
+return QualType();
+  }
+
   if (checkQualifiedFunction(*this, T, Loc, QFK_Pointer))
 return QualType();
 
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -6160,7 +6160,7 @@
 QualType NR = R;
 while (NR->isPointerType()) {
   if (NR->isFunctionPointerType()) {
-Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer_variable);
+Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer);
 D.setInvalidType();
 break;
   }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -7268,7 +7268,7 @@
   "invalid conversion between vector type %0 and integer type %1 "
   "of different size">;
 
-def err_opencl_function_pointer_variable : Error<
+def err_opencl_function_pointer : Error<
   "pointers to functions are not allowed">;
 
 def err_opencl_taking_function_address : Error<


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -4,8 +4,15 @@
 void vararg_f(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
 typedef void (*vararg_fptr_t)(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
+// expected-error@-1{{pointers to functions are not allowed}}
 int printf(__constant const char *st, ...); // expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
 
+// Struct type with function pointer field
+typedef struct s
+{
+   void (*f)(struct s *self, int *i);   // expected-error{{pointers to functions are not allowed}}
+} s_t;
+
 //Function pointer
 void foo(void*);
 
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -1881,6 +1881,11 @@
 return QualType();
   }
 
+  if (T->isFunctionType() && getLangOpts().OpenCL) {
+Diag(Loc, diag::err_opencl_function_pointer);
+return QualType();
+  }
+
   if (checkQualifiedFunction(*this, T, Loc, QFK_Pointer))
 return QualType();
 
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -6160,7 +6160,7 @@
 QualType NR = R;
 while (NR->isPointerType()) {
   if (NR->isFunctionPointerType()) {
-Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer_variable);
+Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer);
 D.setInvalidType();
 break;
   }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -7268,7 +7268,7 @@
   "invalid conversion between vector type %0 and integer type %1 "
   "of different size">;
 
-def err_opencl_function_pointer_variable : Error<
+def err_opencl_function_pointer : Error<
   "pointers to functions are not allowed">;
 
 def err_opencl_taking_function_address : Error<
___

[PATCH] D33821: [OpenCL] Harden function pointer diagnostics.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL304575: [OpenCL] Harden function pointer diagnostics. 
(authored by bader).

Changed prior to commit:
  https://reviews.llvm.org/D33821?vs=101181=101242#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33821

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/SemaOpenCL/func.cl


Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -1881,6 +1881,11 @@
 return QualType();
   }
 
+  if (T->isFunctionType() && getLangOpts().OpenCL) {
+Diag(Loc, diag::err_opencl_function_pointer);
+return QualType();
+  }
+
   if (checkQualifiedFunction(*this, T, Loc, QFK_Pointer))
 return QualType();
 
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -6160,7 +6160,7 @@
 QualType NR = R;
 while (NR->isPointerType()) {
   if (NR->isFunctionPointerType()) {
-Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer_variable);
+Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer);
 D.setInvalidType();
 break;
   }
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7268,7 +7268,7 @@
   "invalid conversion between vector type %0 and integer type %1 "
   "of different size">;
 
-def err_opencl_function_pointer_variable : Error<
+def err_opencl_function_pointer : Error<
   "pointers to functions are not allowed">;
 
 def err_opencl_taking_function_address : Error<
Index: cfe/trunk/test/SemaOpenCL/func.cl
===
--- cfe/trunk/test/SemaOpenCL/func.cl
+++ cfe/trunk/test/SemaOpenCL/func.cl
@@ -4,8 +4,15 @@
 void vararg_f(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
 typedef void (*vararg_fptr_t)(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
+// expected-error@-1{{pointers to 
functions are not allowed}}
 int printf(__constant const char *st, ...); // expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 
+// Struct type with function pointer field
+typedef struct s
+{
+   void (*f)(struct s *self, int *i);   // expected-error{{pointers to 
functions are not allowed}}
+} s_t;
+
 //Function pointer
 void foo(void*);
 


Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -1881,6 +1881,11 @@
 return QualType();
   }
 
+  if (T->isFunctionType() && getLangOpts().OpenCL) {
+Diag(Loc, diag::err_opencl_function_pointer);
+return QualType();
+  }
+
   if (checkQualifiedFunction(*this, T, Loc, QFK_Pointer))
 return QualType();
 
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -6160,7 +6160,7 @@
 QualType NR = R;
 while (NR->isPointerType()) {
   if (NR->isFunctionPointerType()) {
-Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer_variable);
+Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer);
 D.setInvalidType();
 break;
   }
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7268,7 +7268,7 @@
   "invalid conversion between vector type %0 and integer type %1 "
   "of different size">;
 
-def err_opencl_function_pointer_variable : Error<
+def err_opencl_function_pointer : Error<
   "pointers to functions are not allowed">;
 
 def err_opencl_taking_function_address : Error<
Index: cfe/trunk/test/SemaOpenCL/func.cl
===
--- cfe/trunk/test/SemaOpenCL/func.cl
+++ cfe/trunk/test/SemaOpenCL/func.cl
@@ -4,8 +4,15 @@
 void vararg_f(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
 typedef void (*vararg_fptr_t)(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
+// 

[PATCH] D33681: Allow function declaration with empty argument list.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 102319.
bader added a comment.

Update one more test missed in the first version of the patch.

Actually it looks like a bug in Parser.
Clang interprets following statement as variable declaration.

  extern pipe write_only int get_pipe();

So the type of the pipe element is function prototype: `int get_pipe()`.

If I understand the intention correctly, clang is expected to treat this 
expression function declaration with pipe return type, which elements are `int`.

Would it acceptable to commit this patch as is and file another bug on Parser?
I need more time to think how to resolve this ambiguity.


https://reviews.llvm.org/D33681

Files:
  lib/Sema/SemaType.cpp
  test/SemaOpenCL/function-no-args.cl
  test/SemaOpenCL/invalid-pipes-cl2.0.cl


Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
 global pipe int gp;// expected-error {{type '__global read_only 
pipe int' can only be used as a function parameter in OpenCL}}
 global reserve_id_t rid;  // expected-error {{the '__global 
reserve_id_t' type cannot be used to declare a program scope variable}}
 
-extern pipe write_only int get_pipe(); // expected-error {{type '__global 
write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type '__global 
write_only pipe int (void)' can only be used as a function parameter in OpenCL}}
 
 kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error 
{{'reserve_id_t' cannot be used as the type of a kernel parameter}}
 }
Index: test/SemaOpenCL/function-no-args.cl
===
--- /dev/null
+++ test/SemaOpenCL/function-no-args.cl
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL2.0 %s
+// expected-no-diagnostics
+
+global int gi;
+int my_func();
+int my_func() {
+  gi = 2;
+  return gi;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4355,7 +4355,7 @@
 
   FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 
-  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus) {
+  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus  && 
!LangOpts.OpenCL) {
 // Simple void foo(), where the incoming T is the result type.
 T = Context.getFunctionNoProtoType(T, EI);
   } else {


Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
 global pipe int gp;// expected-error {{type '__global read_only pipe int' can only be used as a function parameter in OpenCL}}
 global reserve_id_t rid;  // expected-error {{the '__global reserve_id_t' type cannot be used to declare a program scope variable}}
 
-extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int (void)' can only be used as a function parameter in OpenCL}}
 
 kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error {{'reserve_id_t' cannot be used as the type of a kernel parameter}}
 }
Index: test/SemaOpenCL/function-no-args.cl
===
--- /dev/null
+++ test/SemaOpenCL/function-no-args.cl
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL2.0 %s
+// expected-no-diagnostics
+
+global int gi;
+int my_func();
+int my_func() {
+  gi = 2;
+  return gi;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4355,7 +4355,7 @@
 
   FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 
-  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus) {
+  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus  && !LangOpts.OpenCL) {
 // Simple void foo(), where the incoming T is the result type.
 T = Context.getFunctionNoProtoType(T, EI);
   } else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34235: [OpenCL] Fix OpenCL and SPIR version metadata generation.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.

OpenCL and SPIR version metadata must be generated once per module instead of 
once per mangled global value.


https://reviews.llvm.org/D34235

Files:
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/TargetInfo.cpp
  test/CodeGenOpenCL/spir_version.cl

Index: test/CodeGenOpenCL/spir_version.cl
===
--- test/CodeGenOpenCL/spir_version.cl
+++ test/CodeGenOpenCL/spir_version.cl
@@ -10,17 +10,18 @@
 // RUN: %clang_cc1 %s -triple "amdgcn--amdhsa" -emit-llvm -o - -cl-std=CL2.0 | FileCheck %s --check-prefix=CHECK-AMDGCN-CL20
 
 kernel void foo() {}
+kernel void bar() {}
 
-// CHECK-SPIR-CL10: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL10: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-SPIR-CL10: [[SPIR]] = !{i32 1, i32 2}
-// CHECK-SPIR-CL10: [[OCL]] = !{i32 1, i32 0}
-// CHECK-SPIR-CL12: !opencl.spir.version = !{[[VER:![0-9]+]]}
-// CHECK-SPIR-CL12: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL10-DAG: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
+// CHECK-SPIR-CL10-DAG: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
+// CHECK-SPIR-CL10-DAG: [[SPIR]] = !{i32 1, i32 2}
+// CHECK-SPIR-CL10-DAG: [[OCL]] = !{i32 1, i32 0}
+// CHECK-SPIR-CL12-DAG: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL12-DAG: !opencl.ocl.version = !{[[VER]]}
 // CHECK-SPIR-CL12: [[VER]] = !{i32 1, i32 2}
 
-// CHECK-SPIR-CL20: !opencl.spir.version = !{[[VER:![0-9]+]]}
-// CHECK-SPIR-CL20: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL20-DAG: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL20-DAG: !opencl.ocl.version = !{[[VER]]}
 // CHECK-SPIR-CL20: [[VER]] = !{i32 2, i32 0}
 
 // CHECK-AMDGCN-CL10-NOT: !opencl.spir.version
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -7344,8 +7344,6 @@
 };
 }
 
-static void appendOpenCLVersionMD (CodeGen::CodeGenModule );
-
 void AMDGPUTargetCodeGenInfo::setTargetAttributes(
 const Decl *D,
 llvm::GlobalValue *GV,
@@ -7402,8 +7400,6 @@
 if (NumVGPR != 0)
   F->addFnAttr("amdgpu-num-vgpr", llvm::utostr(NumVGPR));
   }
-
-  appendOpenCLVersionMD(M);
 }
 
 unsigned AMDGPUTargetCodeGenInfo::getOpenCLKernelCallingConv() const {
@@ -8074,8 +8070,6 @@
 public:
   SPIRTargetCodeGenInfo(CodeGen::CodeGenTypes )
 : TargetCodeGenInfo(new DefaultABIInfo(CGT)) {}
-  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-CodeGen::CodeGenModule ) const override;
   unsigned getOpenCLKernelCallingConv() const override;
 };
 
@@ -8090,41 +8084,6 @@
 }
 }
 
-/// Emit SPIR specific metadata: OpenCL and SPIR version.
-void SPIRTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
- CodeGen::CodeGenModule ) const {
-  llvm::LLVMContext  = CGM.getModule().getContext();
-  llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx);
-  llvm::Module  = CGM.getModule();
-  // SPIR v2.0 s2.12 - The SPIR version used by the module is stored in the
-  // opencl.spir.version named metadata.
-  llvm::Metadata *SPIRVerElts[] = {
-  llvm::ConstantAsMetadata::get(
-  llvm::ConstantInt::get(Int32Ty, CGM.getLangOpts().OpenCLVersion / 100)),
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-  Int32Ty, (CGM.getLangOpts().OpenCLVersion / 100 > 1) ? 0 : 2))};
-  llvm::NamedMDNode *SPIRVerMD =
-  M.getOrInsertNamedMetadata("opencl.spir.version");
-  SPIRVerMD->addOperand(llvm::MDNode::get(Ctx, SPIRVerElts));
-  appendOpenCLVersionMD(CGM);
-}
-
-static void appendOpenCLVersionMD(CodeGen::CodeGenModule ) {
-  llvm::LLVMContext  = CGM.getModule().getContext();
-  llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx);
-  llvm::Module  = CGM.getModule();
-  // SPIR v2.0 s2.13 - The OpenCL version used by the module is stored in the
-  // opencl.ocl.version named metadata node.
-  llvm::Metadata *OCLVerElts[] = {
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-  Int32Ty, CGM.getLangOpts().OpenCLVersion / 100)),
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-  Int32Ty, (CGM.getLangOpts().OpenCLVersion % 100) / 10))};
-  llvm::NamedMDNode *OCLVerMD =
-  M.getOrInsertNamedMetadata("opencl.ocl.version");
-  OCLVerMD->addOperand(llvm::MDNode::get(Ctx, OCLVerElts));
-}
-
 unsigned SPIRTargetCodeGenInfo::getOpenCLKernelCallingConv() const {
   return llvm::CallingConv::SPIR_KERNEL;
 }
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -1321,6 +1321,9 @@
   /// Emits target specific Metadata for global declarations.
   void EmitTargetMetadata();
 
+  /// Emits OpenCL specific Metadata e.g. OpenCL version.
+  void EmitOpenCLMetadata();
+
   /// Emit the llvm.gcov metadata used to tell LLVM where 

[PATCH] D34235: [OpenCL] Fix OpenCL and SPIR version metadata generation.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 102789.
bader added a comment.

Applied Akira's comment: re-used cached Int32Ty type.


https://reviews.llvm.org/D34235

Files:
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/TargetInfo.cpp
  test/CodeGenOpenCL/spir_version.cl

Index: test/CodeGenOpenCL/spir_version.cl
===
--- test/CodeGenOpenCL/spir_version.cl
+++ test/CodeGenOpenCL/spir_version.cl
@@ -10,17 +10,18 @@
 // RUN: %clang_cc1 %s -triple "amdgcn--amdhsa" -emit-llvm -o - -cl-std=CL2.0 | FileCheck %s --check-prefix=CHECK-AMDGCN-CL20
 
 kernel void foo() {}
+kernel void bar() {}
 
-// CHECK-SPIR-CL10: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
-// CHECK-SPIR-CL10: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
-// CHECK-SPIR-CL10: [[SPIR]] = !{i32 1, i32 2}
-// CHECK-SPIR-CL10: [[OCL]] = !{i32 1, i32 0}
-// CHECK-SPIR-CL12: !opencl.spir.version = !{[[VER:![0-9]+]]}
-// CHECK-SPIR-CL12: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL10-DAG: !opencl.spir.version = !{[[SPIR:![0-9]+]]}
+// CHECK-SPIR-CL10-DAG: !opencl.ocl.version = !{[[OCL:![0-9]+]]}
+// CHECK-SPIR-CL10-DAG: [[SPIR]] = !{i32 1, i32 2}
+// CHECK-SPIR-CL10-DAG: [[OCL]] = !{i32 1, i32 0}
+// CHECK-SPIR-CL12-DAG: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL12-DAG: !opencl.ocl.version = !{[[VER]]}
 // CHECK-SPIR-CL12: [[VER]] = !{i32 1, i32 2}
 
-// CHECK-SPIR-CL20: !opencl.spir.version = !{[[VER:![0-9]+]]}
-// CHECK-SPIR-CL20: !opencl.ocl.version = !{[[VER]]}
+// CHECK-SPIR-CL20-DAG: !opencl.spir.version = !{[[VER:![0-9]+]]}
+// CHECK-SPIR-CL20-DAG: !opencl.ocl.version = !{[[VER]]}
 // CHECK-SPIR-CL20: [[VER]] = !{i32 2, i32 0}
 
 // CHECK-AMDGCN-CL10-NOT: !opencl.spir.version
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -7344,8 +7344,6 @@
 };
 }
 
-static void appendOpenCLVersionMD (CodeGen::CodeGenModule );
-
 void AMDGPUTargetCodeGenInfo::setTargetAttributes(
 const Decl *D,
 llvm::GlobalValue *GV,
@@ -7402,8 +7400,6 @@
 if (NumVGPR != 0)
   F->addFnAttr("amdgpu-num-vgpr", llvm::utostr(NumVGPR));
   }
-
-  appendOpenCLVersionMD(M);
 }
 
 unsigned AMDGPUTargetCodeGenInfo::getOpenCLKernelCallingConv() const {
@@ -8074,8 +8070,6 @@
 public:
   SPIRTargetCodeGenInfo(CodeGen::CodeGenTypes )
 : TargetCodeGenInfo(new DefaultABIInfo(CGT)) {}
-  void emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
-CodeGen::CodeGenModule ) const override;
   unsigned getOpenCLKernelCallingConv() const override;
 };
 
@@ -8090,41 +8084,6 @@
 }
 }
 
-/// Emit SPIR specific metadata: OpenCL and SPIR version.
-void SPIRTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
- CodeGen::CodeGenModule ) const {
-  llvm::LLVMContext  = CGM.getModule().getContext();
-  llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx);
-  llvm::Module  = CGM.getModule();
-  // SPIR v2.0 s2.12 - The SPIR version used by the module is stored in the
-  // opencl.spir.version named metadata.
-  llvm::Metadata *SPIRVerElts[] = {
-  llvm::ConstantAsMetadata::get(
-  llvm::ConstantInt::get(Int32Ty, CGM.getLangOpts().OpenCLVersion / 100)),
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-  Int32Ty, (CGM.getLangOpts().OpenCLVersion / 100 > 1) ? 0 : 2))};
-  llvm::NamedMDNode *SPIRVerMD =
-  M.getOrInsertNamedMetadata("opencl.spir.version");
-  SPIRVerMD->addOperand(llvm::MDNode::get(Ctx, SPIRVerElts));
-  appendOpenCLVersionMD(CGM);
-}
-
-static void appendOpenCLVersionMD(CodeGen::CodeGenModule ) {
-  llvm::LLVMContext  = CGM.getModule().getContext();
-  llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx);
-  llvm::Module  = CGM.getModule();
-  // SPIR v2.0 s2.13 - The OpenCL version used by the module is stored in the
-  // opencl.ocl.version named metadata node.
-  llvm::Metadata *OCLVerElts[] = {
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-  Int32Ty, CGM.getLangOpts().OpenCLVersion / 100)),
-  llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-  Int32Ty, (CGM.getLangOpts().OpenCLVersion % 100) / 10))};
-  llvm::NamedMDNode *OCLVerMD =
-  M.getOrInsertNamedMetadata("opencl.ocl.version");
-  OCLVerMD->addOperand(llvm::MDNode::get(Ctx, OCLVerElts));
-}
-
 unsigned SPIRTargetCodeGenInfo::getOpenCLKernelCallingConv() const {
   return llvm::CallingConv::SPIR_KERNEL;
 }
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -1321,6 +1321,9 @@
   /// Emits target specific Metadata for global declarations.
   void EmitTargetMetadata();
 
+  /// Emits OpenCL specific Metadata e.g. OpenCL version.
+  void EmitOpenCLMetadata();
+
   /// Emit the llvm.gcov metadata used to tell LLVM where to emit the .gcno 

[PATCH] D33989: [OpenCL] Allow targets to select address space per type

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: include/clang/Basic/TargetInfo.h:1041
+default:
+  return LangAS::Default;
+}

yaxunl wrote:
> bader wrote:
> > yaxunl wrote:
> > > I think the default (including even_t, clk_event_t, queue_t, 
> > > reserved_id_t) should be global since these opaque OpenCL objects are 
> > > pointers to some shared resources. These pointers may be an automatic 
> > > variable themselves but the memory they point to should be global. Since 
> > > these objects are dynamically allocated, assuming them in private address 
> > > space implies runtime needs to maintain a private memory pool for each 
> > > work item and allocate objects from there. Considering the huge number of 
> > > work items in typical OpenCL applications, it would be very inefficient 
> > > to implement these objects in private memory pool. On the other hand, a 
> > > global memory pool seems much reasonable.
> > > 
> > > Anastasia/Alexey, any comments on this? Thanks.
> > I remember we discussed this a couple of time in the past. 
> > The address space for variables of these types is not clearly stated in the 
> > spec, so I think the right way to treat it - it's implementation defined.
> > On the other hand your reasoning on using global address space as default 
> > AS makes sense in general - so can we put additional clarification to the 
> > spec to align it with the proposed implementation?
> I think it is unnecessary to specify the implementation details in the OpenCL 
> spec.
> 
> It is also unnecessary for SPIR-V spec since the pointee address space of 
> OpenCL opaque struct is not encoded in SPIR-V.
> 
> We can use the commonly accepted address space definition in the TargetInfo 
> base class. If a target chooses to use different address space for specific 
> opaque objects, it can override it in its own virtual function.
> I think it is unnecessary to specify the implementation details in the OpenCL 
> spec.

Agree, but my point was about specifying the intention in the specification.
For instance, OpenCL spec says that image objects are located in global memory.
It says nothing about objects like events, queues, etc., so I assumed that if 
it says nothing an implementation is allowed to choose the memory region, but 
it might be false assumption.


https://reviews.llvm.org/D33989



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

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.

Constant samplers are handled as static variables and clang's code generation
library, which leads to llvm::unreachable. We bypass emitting sampler variable
as static since it's translated to a function call later.


https://reviews.llvm.org/D34342

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenOpenCL/sampler.cl


Index: test/CodeGenOpenCL/sampler.cl
===
--- test/CodeGenOpenCL/sampler.cl
+++ test/CodeGenOpenCL/sampler.cl
@@ -58,4 +58,11 @@
   fnc4smp(5);
   // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 5)
   // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
+
+  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 35)
+  // CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], 
%opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]]
+  fnc4smp(const_smp);
+  // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, 
%opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]]
+  // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
 }
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -161,6 +161,10 @@
   // needs to be emitted like a static variable, e.g. a function-scope
   // variable in constant address space in OpenCL.
   if (D.getStorageDuration() != SD_Automatic) {
+// Static sampler variables translated to function calls.
+if (D.getType()->isSamplerT())
+  return;
+
 llvm::GlobalValue::LinkageTypes Linkage =
 CGM.getLLVMLinkageVarDefinition(, /*isConstant=*/false);
 


Index: test/CodeGenOpenCL/sampler.cl
===
--- test/CodeGenOpenCL/sampler.cl
+++ test/CodeGenOpenCL/sampler.cl
@@ -58,4 +58,11 @@
   fnc4smp(5);
   // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 5)
   // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
+
+  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35)
+  // CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]]
+  fnc4smp(const_smp);
+  // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]]
+  // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
 }
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -161,6 +161,10 @@
   // needs to be emitted like a static variable, e.g. a function-scope
   // variable in constant address space in OpenCL.
   if (D.getStorageDuration() != SD_Automatic) {
+// Static sampler variables translated to function calls.
+if (D.getType()->isSamplerT())
+  return;
+
 llvm::GlobalValue::LinkageTypes Linkage =
 CGM.getLLVMLinkageVarDefinition(, /*isConstant=*/false);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33945: [OpenCL] Add support for missing sub_group functions.

[Alexey Bader via Phabricator via cfe-commits]

bader requested changes to this revision.
bader added a comment.
This revision now requires changes to proceed.

Please, split this patch into two parts:

1. Improve diagnostics on extension enabling.
2. Add missing `sub_group_*` built-in functions.




Comment at: Sema/SemaChecking.cpp:1091-1092
   case Builtin::BIwork_group_reserve_write_pipe:
+if (SemaBuiltinReserveRWPipe(*this, TheCall, false))
+  return ExprError();
+// Since return type of reserve_read/write_pipe built-in function is

I suggest leaving `SemaBuiltinReserveRWPipe` as is (i.e. two parameter) and 
modify the check for `sub_group_*` functions only:
```
if (checkOpenCLSubgroupExt(S, TheCall) || SemaBuiltinReserveRWPipe(*this, 
TheCall))
  return ExprError();
```

It think it's more readable than looking at SemaBuiltinReserveRWPipe 
declaration or leaving the comment like this:
```
if (SemaBuiltinReserveRWPipe(*this, TheCall, false /*isSubgroup*/))
```

Please, apply the same approach to `SemaBuiltinCommitRWPipe`.

In addition to that, it makes sense to set the `TheCall` type inside the 
`SemaBuiltinReserveRWPipe` to avoid code duplication.



Comment at: SemaOpenCL/cl20-device-side-enqueue.cl:219
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected 'ndrange_t' argument type}}
+}

Please, add a test case(s) on invalid block parameters to cover the checks you 
added for new `sub_group_` built-ins..



Comment at: SemaOpenCL/sub-group-bifs.cl:1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -cl-std=CL2.0
+

I don't think it makes sense to add this test. This test look like a duplicate 
of test cases added to SemaOpenCL/cl20-device-side-enqueue.cl.




Comment at: clang/Basic/Builtins.def:1402-1403
 LANGBUILTIN(get_kernel_preferred_work_group_size_multiple, "i.", "tn", 
OCLC20_LANG)
+LANGBUILTIN(get_kernel_max_sub_group_size_for_ndrange, "i.", "tn", OCLC20_LANG)
+LANGBUILTIN(get_kernel_sub_group_count_for_ndrange, "i.", "tn", OCLC20_LANG)
 

This built-in functions should return unsigned integers: "i." -> "Ui.".
Please, fix `get_kernel_work_group_size` and 
`get_kernel_preferred_work_group_size_multiple` too.



Comment at: clang/Basic/DiagnosticSemaKinds.td:8423-8424
   "illegal call to enqueue_kernel, incorrect argument types">;
 def err_opencl_enqueue_kernel_expected_type : Error<
-  "illegal call to enqueue_kernel, expected %0 argument type">;
+  "illegal call to %0, expected %1 argument type">;
 def err_opencl_enqueue_kernel_local_size_args : Error<

Since, this message is not specific to enqueue_kernel anymore, I suggest either 
rename it or better re-use existing diagnostics if possible. I think there are 
already message to report argument mismatch + probably additional note 
diagnostics that hints correct argument type.


https://reviews.llvm.org/D33945



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

[PATCH] D33989: [OpenCL] Allow targets to select address space per type

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: include/clang/Basic/TargetInfo.h:1041
+default:
+  return LangAS::Default;
+}

yaxunl wrote:
> I think the default (including even_t, clk_event_t, queue_t, reserved_id_t) 
> should be global since these opaque OpenCL objects are pointers to some 
> shared resources. These pointers may be an automatic variable themselves but 
> the memory they point to should be global. Since these objects are 
> dynamically allocated, assuming them in private address space implies runtime 
> needs to maintain a private memory pool for each work item and allocate 
> objects from there. Considering the huge number of work items in typical 
> OpenCL applications, it would be very inefficient to implement these objects 
> in private memory pool. On the other hand, a global memory pool seems much 
> reasonable.
> 
> Anastasia/Alexey, any comments on this? Thanks.
I remember we discussed this a couple of time in the past. 
The address space for variables of these types is not clearly stated in the 
spec, so I think the right way to treat it - it's implementation defined.
On the other hand your reasoning on using global address space as default AS 
makes sense in general - so can we put additional clarification to the spec to 
align it with the proposed implementation?



Comment at: lib/Basic/Targets.cpp:2367
 
-  LangAS::ID getOpenCLImageAddrSpace() const override {
+  virtual LangAS::ID
+  getOpenCLTypeAddrSpace(BuiltinType::Kind K) const override {

I think the rule LLVM applies is: use virtual in base classes and override w/o 
virtual in derived classes.
'virtual' is not necessary here.


https://reviews.llvm.org/D33989



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

[PATCH] D37804: [OpenCL] Handle address space conversion while setting type alignment

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/CodeGenOpenCL/vectorLoadStore.cl:7
+typedef float float4 __attribute((ext_vector_type(4)));
+;
 

Can we remove this line?



Comment at: test/CodeGenOpenCL/vectorLoadStore.cl:15
+
+// CHECK: define spir_func void @alignment()
+void alignment() {

Please, set SPIR target in clang options to make this check reliable.


https://reviews.llvm.org/D37804



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

[PATCH] D33681: [OpenCL] Allow function declaration with empty argument list.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 118210.
bader edited the summary of this revision.
bader added a comment.

Fix parsing of function declarations with empty parameter list initializers.
This change should fix the failure caught by the buildbot.
Sorry for the long delay.


https://reviews.llvm.org/D33681

Files:
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaType.cpp
  test/SemaOpenCL/function-no-args.cl
  test/SemaOpenCL/invalid-pipes-cl2.0.cl


Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
 global pipe int gp;// expected-error {{type '__global read_only 
pipe int' can only be used as a function parameter in OpenCL}}
 global reserve_id_t rid;  // expected-error {{the '__global 
reserve_id_t' type cannot be used to declare a program scope variable}}
 
-extern pipe write_only int get_pipe(); // expected-error {{type '__global 
write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type '__global 
write_only pipe int (void)' can only be used as a function parameter in OpenCL}}
 
 kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error 
{{'reserve_id_t' cannot be used as the type of a kernel parameter}}
 }
Index: test/SemaOpenCL/function-no-args.cl
===
--- test/SemaOpenCL/function-no-args.cl
+++ test/SemaOpenCL/function-no-args.cl
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL2.0 %s
+// expected-no-diagnostics
+
+global int gi;
+int my_func();
+int my_func() {
+  gi = 2;
+  return gi;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4460,7 +4460,8 @@
 
   FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 
-  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus) {
+  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus
+&& !LangOpts.OpenCL) {
 // Simple void foo(), where the incoming T is the result type.
 T = Context.getFunctionNoProtoType(T, EI);
   } else {
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -5989,7 +5989,8 @@
 else if (RequiresArg)
   Diag(Tok, diag::err_argument_required_after_attribute);
 
-HasProto = ParamInfo.size() || getLangOpts().CPlusPlus;
+HasProto = ParamInfo.size() || getLangOpts().CPlusPlus
+|| getLangOpts().OpenCL;
 
 // If we have the closing ')', eat it.
 Tracker.consumeClose();


Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
 global pipe int gp;// expected-error {{type '__global read_only pipe int' can only be used as a function parameter in OpenCL}}
 global reserve_id_t rid;  // expected-error {{the '__global reserve_id_t' type cannot be used to declare a program scope variable}}
 
-extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int (void)' can only be used as a function parameter in OpenCL}}
 
 kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error {{'reserve_id_t' cannot be used as the type of a kernel parameter}}
 }
Index: test/SemaOpenCL/function-no-args.cl
===
--- test/SemaOpenCL/function-no-args.cl
+++ test/SemaOpenCL/function-no-args.cl
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL2.0 %s
+// expected-no-diagnostics
+
+global int gi;
+int my_func();
+int my_func() {
+  gi = 2;
+  return gi;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4460,7 +4460,8 @@
 
   FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 
-  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus) {
+  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus
+&& !LangOpts.OpenCL) {
 // Simple void foo(), where the incoming T is the result type.
 T = Context.getFunctionNoProtoType(T, EI);
   } else {
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ 

[PATCH] D33681: [OpenCL] Allow function declaration with empty argument list.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 118437.
bader added a comment.

Applied comments.


https://reviews.llvm.org/D33681

Files:
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaType.cpp
  test/SemaOpenCL/func.cl
  test/SemaOpenCL/invalid-pipes-cl2.0.cl


Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
 global pipe int gp;// expected-error {{type '__global read_only 
pipe int' can only be used as a function parameter in OpenCL}}
 global reserve_id_t rid;  // expected-error {{the '__global 
reserve_id_t' type cannot be used to declare a program scope variable}}
 
-extern pipe write_only int get_pipe(); // expected-error {{type '__global 
write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type '__global 
write_only pipe int (void)' can only be used as a function parameter in OpenCL}}
 
 kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error 
{{'reserve_id_t' cannot be used as the type of a kernel parameter}}
 }
Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple 
spir-unknown-unknown
 
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
@@ -16,6 +16,9 @@
 //Function pointer
 void foo(void*);
 
+// Expect no diagnostics for an empty parameter list.
+void bar();
+
 void bar()
 {
   // declaring a function pointer is an error
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4460,7 +4460,8 @@
 
   FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 
-  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus) {
+  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus
+&& !LangOpts.OpenCL) {
 // Simple void foo(), where the incoming T is the result type.
 T = Context.getFunctionNoProtoType(T, EI);
   } else {
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -5989,7 +5989,8 @@
 else if (RequiresArg)
   Diag(Tok, diag::err_argument_required_after_attribute);
 
-HasProto = ParamInfo.size() || getLangOpts().CPlusPlus;
+HasProto = ParamInfo.size() || getLangOpts().CPlusPlus
+|| getLangOpts().OpenCL;
 
 // If we have the closing ')', eat it.
 Tracker.consumeClose();


Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
 global pipe int gp;// expected-error {{type '__global read_only pipe int' can only be used as a function parameter in OpenCL}}
 global reserve_id_t rid;  // expected-error {{the '__global reserve_id_t' type cannot be used to declare a program scope variable}}
 
-extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int (void)' can only be used as a function parameter in OpenCL}}
 
 kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error {{'reserve_id_t' cannot be used as the type of a kernel parameter}}
 }
Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple spir-unknown-unknown
 
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
@@ -16,6 +16,9 @@
 //Function pointer
 void foo(void*);
 
+// Expect no diagnostics for an empty parameter list.
+void bar();
+
 void bar()
 {
   // declaring a function pointer is an error
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4460,7 +4460,8 @@
 
   FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 
-  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus) {
+  if (!FTI.NumParams && !FTI.isVariadic && !LangOpts.CPlusPlus

[PATCH] D38816: Convert clang::LangAS to a strongly typed enum

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

@arichardson, great work! Thanks a lot!


https://reviews.llvm.org/D38816



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

[PATCH] D36410: [OpenCL] Handle taking address of block captures

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D36410#856716, @yaxunl wrote:

> The captured variable is still passed by value. The address taking is on the 
> duplicate of the captured variable, not on the original variable.


In this case address of captured variables should point to the private address 
space, as function parameters reside in private address space (see "6.5 Address 
Space Qualifiers" paragraph 3).
This makes unclear to me how capturing works for variables declared in the 
address spaces other than private (e.g. `local int a;`).

> The OpenCL spec does not forbid taking address of captured variable. 
> Forbidding taking address of captured variable would violate the spec.

I'm not sure yet, but it might be OpenCL spec inconsistency issue.


https://reviews.llvm.org/D36410



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

[PATCH] D37804: [OpenCL] Handle address space conversion while setting type alignment

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.

LGTM. Thanks!


https://reviews.llvm.org/D37804



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

[PATCH] D38463: [OpenCL] Fix checking of vector type casting

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.


https://reviews.llvm.org/D38463



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

[PATCH] D36951: [OpenCL][5.0.0 Release] Release notes for OpenCL in Clang

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM, except one nit.




Comment at: docs/ReleaseNotes.rst:146
+-  Extended Clang builtins with required ``cl_khr_subgroups`` support.
+-  Now interpreting empty argument function as void argument list in OpenCL 
code.
+-  Add ``intel_reqd_sub_group_size`` attribute support.

As far as I remember, this change was reverted as it had caused some 
regressions. I haven't fix them yet, so I users still have to explicitly 
specify void for empty parameter list.


https://reviews.llvm.org/D36951



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

[PATCH] D36410: [OpenCL] Handle taking address of block captures

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D36410#855282, @Anastasia wrote:

> Ok, I will update it to be implicitly generic then. Just to be sure, @bader 
> do you agree on this too?




> An alternative approached could be (in case we want to allow this code) to 
> **assume captures are located in the generic AS implicitly**. However, in 
> this case programmers should be advised that erroneous AS casts can occur 
> further that can't be diagnosed by the frontend (i.e. if capture address is 
> used further in the operations of a different address space to where the 
> captures physical location is).

I don't have a strong opinion on this topic, but I'm worried about relying on 
implicit assumptions, which might make code less portable.

How the alternative approach is this suppose to work for enqueue_kernel? Do we 
assume that the captured variables passed by generic pointer and compiler will 
assign the correct address space based on the explicit casts in the block?
There are some restrictions on kernel parameters: 6.9.a. Arguments to kernel 
functions declared in a program that are pointers must be declared with the 
global, constant or local qualifier.

> I feel forbidding user taking address of captured variable is too restrictive 
> and make blocks less useful.

I have no information how widely blocks are used by the OpenCL community and 
how important this feature is.


https://reviews.llvm.org/D36410



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

[PATCH] D39936: [OpenCL] Add extensions cl_intel_subgroups and cl_intel_subgroups_short

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.

LGTM. Thanks!


https://reviews.llvm.org/D39936



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

[PATCH] D39129: [OpenCL] Fix generation of constant address space sampler in function scope

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D39129#923235, @Anastasia wrote:

> In https://reviews.llvm.org/D39129#902848, @bader wrote:
>
> > @Anastasia, during the discussion of similar fix 
> > (https://reviews.llvm.org/D34342).
> >
> > I found another bug in the CodeGen library. Do you have time to fix it too?
> >  Here is the reproducer from the old review request:
> >
> >   int get_sampler_initializer(void);
> >   kernel void foo() {
> > const sampler_t const_smp_func_init = get_sampler_initializer();
> >   }
> >
>
>
> I am getting an error currently: `error: internal error: could not emit 
> constant value "abstractly"`. It seems very weird and also doesn't seem to be 
> related to constant AS change.  I think this should be allowed to compile 
> though? I can try to investigate but since I don't know the time frame yet 
> and it doesn't seem to be related to constant AS I would prefer to do it as a 
> separate change.


I'm fine with fixing this test case later, but I assume that patch posted 
https://reviews.llvm.org/D34342 covers more cases. Is this version able to 
handle samples declared as `const sampler_t const_smp`?
Are you okay if I commit https://reviews.llvm.org/D34342 instead?


https://reviews.llvm.org/D39129



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

[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318290: [OpenCL] Fix code generation of function-scope 
constant samplers. (authored by bader).

Repository:
  rL LLVM

https://reviews.llvm.org/D34342

Files:
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/test/CodeGenOpenCL/sampler.cl
  cfe/trunk/test/SemaOpenCL/sampler_t.cl


Index: cfe/trunk/lib/CodeGen/CGDecl.cpp
===
--- cfe/trunk/lib/CodeGen/CGDecl.cpp
+++ cfe/trunk/lib/CodeGen/CGDecl.cpp
@@ -162,6 +162,10 @@
   // needs to be emitted like a static variable, e.g. a function-scope
   // variable in constant address space in OpenCL.
   if (D.getStorageDuration() != SD_Automatic) {
+// Static sampler variables translated to function calls.
+if (D.getType()->isSamplerT())
+  return;
+
 llvm::GlobalValue::LinkageTypes Linkage =
 CGM.getLLVMLinkageVarDefinition(, /*isConstant=*/false);
 
Index: cfe/trunk/test/CodeGenOpenCL/sampler.cl
===
--- cfe/trunk/test/CodeGenOpenCL/sampler.cl
+++ cfe/trunk/test/CodeGenOpenCL/sampler.cl
@@ -20,6 +20,8 @@
 constant sampler_t glb_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
 // CHECK-NOT: glb_smp
 
+int get_sampler_initializer(void);
+
 void fnc4smp(sampler_t s) {}
 // CHECK: define spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* %
 
@@ -58,4 +60,20 @@
   fnc4smp(5);
   // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 5)
   // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
+
+  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  fnc4smp(const_smp);
+   // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 35)
+  // CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], 
%opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]]
+  fnc4smp(const_smp);
+  // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, 
%opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]]
+  // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
+
+  constant sampler_t constant_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  fnc4smp(constant_smp);
+  // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* 
@__translate_sampler_initializer(i32 35)
+  // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* 
[[SAMP]])
+
+  // TODO: enable sampler initialization with non-constant integer.
+  //const sampler_t const_smp_func_init = get_sampler_initializer();
 }
Index: cfe/trunk/test/SemaOpenCL/sampler_t.cl
===
--- cfe/trunk/test/SemaOpenCL/sampler_t.cl
+++ cfe/trunk/test/SemaOpenCL/sampler_t.cl
@@ -46,36 +46,11 @@
 
 void kernel ker(sampler_t argsmp) {
   local sampler_t smp; // expected-error{{sampler type cannot be used with the 
__local and __global address space qualifiers}}
-  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | 
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
-  const sampler_t const_smp2;
-  const sampler_t const_smp3 = const_smp;
-  const sampler_t const_smp4 = f();
   const sampler_t const_smp5 = 1.0f; // expected-error{{initializing 'const 
sampler_t' with an expression of incompatible type 'float'}}
   const sampler_t const_smp6 = 0x1LL; // expected-error{{sampler_t 
initialization requires 32-bit integer, not 'long long'}}
 
-  foo(glb_smp);
-  foo(glb_smp2);
-  foo(glb_smp3);
-  foo(glb_smp4);
-  foo(glb_smp5);
-  foo(glb_smp6);
-  foo(glb_smp7);
-  foo(glb_smp8);
-  foo(glb_smp9);
-  foo(smp);
-  foo(sampler_str.smp);
-  foo(const_smp);
-  foo(const_smp2);
-  foo(const_smp3);
-  foo(const_smp4);
-  foo(const_smp5);
-  foo(const_smp6);
-  foo(argsmp);
-  foo(5);
   foo(5.0f); // expected-error {{passing 'float' to parameter of incompatible 
type 'sampler_t'}}
-  sampler_t sa[] = {argsmp, const_smp}; // expected-error {{array of 
'sampler_t' type is invalid in OpenCL}}
-  foo(sa[0]);
-  foo(bad());
+  sampler_t sa[] = {argsmp, glb_smp}; // expected-error {{array of 'sampler_t' 
type is invalid in OpenCL}}
 }
 
 void bad(sampler_t*); // expected-error{{pointer to type 'sampler_t' is 
invalid in OpenCL}}


Index: cfe/trunk/lib/CodeGen/CGDecl.cpp
===
--- cfe/trunk/lib/CodeGen/CGDecl.cpp
+++ cfe/trunk/lib/CodeGen/CGDecl.cpp
@@ -162,6 +162,10 @@
   // needs to be emitted like a static variable, e.g. a function-scope
   // variable in constant address space in OpenCL.
   if (D.getStorageDuration() != SD_Automatic) {
+// Static sampler variables translated to function calls.
+if (D.getType()->isSamplerT())
+  return;
+
 

[PATCH] D39129: [OpenCL] Fix generation of constant address space sampler in function scope

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

@Anastasia, during the discussion of similar fix 
(https://reviews.llvm.org/D34342).

I found another bug in the CodeGen library. Do you have time to fix it too?
Here is the reproducer from the old review request:

  int get_sampler_initializer(void);
  kernel void foo() {
const sampler_t const_smp_func_init = get_sampler_initializer();
  }


https://reviews.llvm.org/D39129



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

[PATCH] D46049: [OpenCL] Add constant address space to __func__ in AST

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/AST/Expr.cpp:870
+  if (C.getLangOpts().OpenCL && Ty.getAddressSpace() == LangAS::Default)
+Ty = C.getAddrSpaceQualType(Ty, LangAS::opencl_constant);
+

Anastasia wrote:
> bader wrote:
> > As `Ty` is passed by value, shouldn't we accept only data located in 
> > constant address space?
> Do you mean to assert? Currently it should be passed with `constant` AS but I 
> thought the idea is to modify this function so we can accept `Default` AS too 
> but then replace by `constant`.
> 
Yes, but according to your other comment this idea doesn't work.

> I have added the address space to the creation of StringLiteral, but 
> unfortunately it doesn't seems like we can remove similar code in other 
> places because **QualType created for StringLiteral is also used elsewhere 
> and has to match (contain right address space).** I.e. here is it used 
> further down to create PredefinedExpr.






https://reviews.llvm.org/D46049



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

[PATCH] D46501: [OpenCL] Guard all half float usage based on cl_khr_fp16 extension

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Headers/opencl-c.h:12097
+#ifdef cl_khr_fp16
 float __ovld vload_half(size_t offset, const __constant half *p);
 #if __OPENCL_C_VERSION__ >= CL_VERSION_2_0

These built-ins are part of the core specification, which doesn't allow using 
`half` data type directly, but user can declare a pointer to `half` data type 
and use built-ins like vload_half/vstore_half converting `half` data type to 
`float` data type.

cl_khr_fp16 enables regular uses of half data types as well as built-ins 
returning `half` data type instead of `float` - vload/vstore.


Repository:
  rC Clang

https://reviews.llvm.org/D46501



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

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/Sema/address_spaces.c:17
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified 
for type}}
+  int *_AS1 _AS1 *M;
 

ebevhan wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > bader wrote:
> > > > I think it might be valuable to give a warning or remark to user. 
> > > > Using the same address space qualifier multiple times is not something 
> > > > OpenCL C developers are supposed to do.
> > > > 
> > > The test is obviously a bit contrived, but it could happen by mistake, or 
> > > as a result of some typedef or macro combination. It also cannot go 
> > > wrong, so there's no harm in it happening.
> > > 
> > > I see your point, though. A warning feels like a bit much, so I'm not 
> > > sure what else to use. A note?
> > Just checked for const qualifier we get a warning:
> >   warning: duplicate 'const' declaration specifier 
> > [-Wduplicate-decl-specifier]
> > 
> > We could do the same... not sure if we could try to share the diagnostic as 
> > well.
> I have a patch ready that adds a warning in the same warning group and uses 
> that. I'm not sure about reusing the other one since address spaces don't 
> have to be declaration specifiers.
> 
> The warning is 'multiple identical address spaces specified for type', 
> similar to the error. Is that acceptable?
Sounds good to me. Could you share your patch, please?


Repository:
  rC Clang

https://reviews.llvm.org/D47630



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

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/Sema/address_spaces.c:17
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified 
for type}}
+  int *_AS1 _AS1 *M;
 

I think it might be valuable to give a warning or remark to user. 
Using the same address space qualifier multiple times is not something OpenCL C 
developers are supposed to do.



Repository:
  rC Clang

https://reviews.llvm.org/D47630



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

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC335103: [Sema] Allow creating types with multiple of the 
same addrspace. (authored by bader, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47630?vs=150683=152033#toc

Repository:
  rC Clang

https://reviews.llvm.org/D47630

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  test/Sema/address_spaces.c
  test/SemaOpenCL/address-spaces.cl

Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -62,4 +62,6 @@
   __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
+  __private private_int_t var5; // expected-warning {{multiple identical address spaces specified for type}}
+  __private private_int_t *var6;// expected-warning {{multiple identical address spaces specified for type}}
 }
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -14,6 +14,7 @@
 
   int _AS1 _AS2 *Y;   // expected-error {{multiple address spaces specified for type}}
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified for type}}
+  int *_AS1 _AS1 *M;  // expected-warning {{multiple identical address spaces specified for type}}
 
   _AS1 int local; // expected-error {{automatic variable qualified with an address space}}
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an address space}}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5758,14 +5758,6 @@
  SourceLocation AttrLoc) {
   if (!AddrSpace->isValueDependent()) { 
 
-// If this type is already address space qualified, reject it.
-// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
-// by qualifiers for two or more different address spaces."
-if (T.getAddressSpace() != LangAS::Default) {
-  Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
-  return QualType();
-}
-
 llvm::APSInt addrSpace(32);
 if (!AddrSpace->isIntegerConstantExpr(addrSpace, Context)) {
   Diag(AttrLoc, diag::err_attribute_argument_type)
@@ -5796,6 +5788,20 @@
 LangAS ASIdx =
 getLangASFromTargetAS(static_cast(addrSpace.getZExtValue()));
 
+// If this type is already address space qualified with a different
+// address space, reject it.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+if (T.getAddressSpace() != LangAS::Default) {
+  if (T.getAddressSpace() != ASIdx) {
+Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+return QualType();
+  } else
+// Emit a warning if they are identical; it's likely unintended.
+Diag(AttrLoc,
+ diag::warn_attribute_address_multiple_identical_qualifiers);
+}
+
 return Context.getAddrSpaceQualType(T, ASIdx);
   }
 
@@ -5817,15 +5823,6 @@
 /// space for the type.
 static void HandleAddressSpaceTypeAttribute(QualType ,
 const AttributeList , Sema ){
-  // If this type is already address space qualified, reject it.
-  // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
-  // qualifiers for two or more different address spaces."
-  if (Type.getAddressSpace() != LangAS::Default) {
-S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
-Attr.setInvalid();
-return;
-  }
-
   // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
   // qualified by an address-space qualifier."
   if (Type->isFunctionType()) {
@@ -5888,6 +5885,21 @@
   llvm_unreachable("Invalid address space");
 }
 
+// If this type is already address space qualified with a different
+// address space, reject it.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
+// qualifiers for two or more different address spaces."
+if (Type.getAddressSpace() != LangAS::Default) {
+  if (Type.getAddressSpace() != ASIdx) {
+S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
+Attr.setInvalid();
+return;
+  } else
+// Emit a warning if they are identical; it's likely unintended.
+S.Diag(Attr.getLoc(),
+   diag::warn_attribute_address_multiple_identical_qualifiers);
+}
+
 Type = 

[PATCH] D46049: [OpenCL] Add constant address space to __func__ in AST

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Sema/SemaExpr.cpp:3056
+  if (LangOpts.OpenCL)
+ResTy = Context.getAddrSpaceQualType(ResTy, LangAS::opencl_constant);
   ResTy = Context.getConstantArrayType(ResTy, LengthI, ArrayType::Normal,

Do we still need this?



https://reviews.llvm.org/D46049



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

[PATCH] D42307: [OpenCL][6.0.0 Release] Release notes for OpenCL in Clang

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.

Looks good to me. Thanks for working on this.


https://reviews.llvm.org/D42307



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

[PATCH] D42532: [OpenCL] Add "cles_khr_int64" extension.

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL323522: [OpenCL] Add cles_khr_int64 extension. 
(authored by bader, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D42532

Files:
  cfe/trunk/include/clang/Basic/OpenCLExtensions.def
  cfe/trunk/test/SemaOpenCL/extension-version.cl


Index: cfe/trunk/include/clang/Basic/OpenCLExtensions.def
===
--- cfe/trunk/include/clang/Basic/OpenCLExtensions.def
+++ cfe/trunk/include/clang/Basic/OpenCLExtensions.def
@@ -53,6 +53,9 @@
 OPENCLEXT_INTERNAL(cl_khr_gl_event, 110, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_d3d10_sharing, 110, ~0U)
 
+// EMBEDDED_PROFILE
+OPENCLEXT_INTERNAL(cles_khr_int64, 110, ~0U)
+
 // OpenCL 1.2.
 OPENCLEXT_INTERNAL(cl_khr_context_abort, 120, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_d3d11_sharing, 120, ~0U)
Index: cfe/trunk/test/SemaOpenCL/extension-version.cl
===
--- cfe/trunk/test/SemaOpenCL/extension-version.cl
+++ cfe/trunk/test/SemaOpenCL/extension-version.cl
@@ -131,6 +131,15 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_d3d10_sharing: enable
 
+#if (__OPENCL_C_VERSION__ >= 110)
+#ifndef cles_khr_int64
+#error "Missing cles_khr_int64 define"
+#endif
+#else
+// expected-warning@+2{{unsupported OpenCL extension 'cles_khr_int64' - 
ignoring}}
+#endif
+#pragma OPENCL EXTENSION cles_khr_int64: enable
+
 #if (__OPENCL_C_VERSION__ >= 120)
 #ifndef cl_khr_context_abort
 #error "Missing cl_context_abort define"


Index: cfe/trunk/include/clang/Basic/OpenCLExtensions.def
===
--- cfe/trunk/include/clang/Basic/OpenCLExtensions.def
+++ cfe/trunk/include/clang/Basic/OpenCLExtensions.def
@@ -53,6 +53,9 @@
 OPENCLEXT_INTERNAL(cl_khr_gl_event, 110, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_d3d10_sharing, 110, ~0U)
 
+// EMBEDDED_PROFILE
+OPENCLEXT_INTERNAL(cles_khr_int64, 110, ~0U)
+
 // OpenCL 1.2.
 OPENCLEXT_INTERNAL(cl_khr_context_abort, 120, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_d3d11_sharing, 120, ~0U)
Index: cfe/trunk/test/SemaOpenCL/extension-version.cl
===
--- cfe/trunk/test/SemaOpenCL/extension-version.cl
+++ cfe/trunk/test/SemaOpenCL/extension-version.cl
@@ -131,6 +131,15 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_d3d10_sharing: enable
 
+#if (__OPENCL_C_VERSION__ >= 110)
+#ifndef cles_khr_int64
+#error "Missing cles_khr_int64 define"
+#endif
+#else
+// expected-warning@+2{{unsupported OpenCL extension 'cles_khr_int64' - ignoring}}
+#endif
+#pragma OPENCL EXTENSION cles_khr_int64: enable
+
 #if (__OPENCL_C_VERSION__ >= 120)
 #ifndef cl_khr_context_abort
 #error "Missing cl_context_abort define"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42532: [OpenCL] Add "cles_khr_int64" extension.

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM, but I'd like Anastasia to approve.
Thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D42532



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

[PATCH] D50259: [OpenCL] Disallow negative attribute arguments

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.

LGTM.
Please, remove "Change-Id: I910b5c077f5f29e02a1572d9202f0fdbea5280fd" from the 
log message - it not relevant to the project.


Repository:
  rC Clang

https://reviews.llvm.org/D50259



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

[PATCH] D43783: [OpenCL] Remove block invoke function from emitted block literal struct

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

Hi Sam,

Sorry for the delay. LGTM, I have only minor refactoring suggestion.

Thanks,
Alexey




Comment at: lib/CodeGen/CGBlocks.cpp:1065-1067
+  llvm::Value *FuncPtr;
 
+  if (!CGM.getLangOpts().OpenCL) {

I think it would be more readable if we merge this if statement with the if 
statement at the line #1103.
It's used to initialize FuncPtr for non-OpenCL languages and the first use of 
this variable is in the else block of if statement at the line #1103.
If I didn't miss something it should reasonable to combine this if block with 
'else' block at the line #1106.


https://reviews.llvm.org/D43783



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

[PATCH] D45363: [OpenCL] Added -std/-cl-std=CL2.2/CLC++

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


https://reviews.llvm.org/D45363



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

[PATCH] D45363: [OpenCL] Added -std/-cl-std=CL2.2/CLC++

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: include/clang/Frontend/LangStandards.def:167
+LANGSTANDARD_ALIAS_DEPR(opencl22, "CL2.2")
+LANGSTANDARD_ALIAS_DEPR(opencl22, "clc++")
+LANGSTANDARD_ALIAS_DEPR(opencl22, "CLC++")

OpenCL C++ 1.0 specification defines only 'c++' value for -cl-std option.
Assuming that OpenCL C++ 1.0 might be supported by multiple OpenCL version, 
wouldn't 'CL2.2' confuse people?
As we have separate kernel language specification with independent version 
numbering, I think we should avoid using OpenCL API numbering for language 
version.

What is the naming scheme for next versions of OpenCL C++ specification? 
'clc++N.M'?



Comment at: lib/Frontend/InitPreprocessor.cpp:428
   // OpenCL v1.0/1.1 s6.9, v1.2/2.0 s6.10: Preprocessor Directives and Macros.
-  if (LangOpts.OpenCL) {
+  if (LangOpts.OpenCL && !LangOpts.CPlusPlus) {
 // OpenCL v1.0 and v1.1 do not have a predefined macro to indicate the

I think we should not skip this section for OpenCL C++, but define 
OPENCL_CPP_VERSION macro.


https://reviews.llvm.org/D45363



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

[PATCH] D46049: [OpenCL] Add constant address space to __func__ in AST

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:3059
/*IndexTypeQuals*/ 0);
   SL = StringLiteral::Create(Context, Str, StringLiteral::Ascii,
  /*Pascal*/ false, ResTy, Loc);

Will it work if we fix this issue inside StringLiteral::Create method?
I just hope it will help us avoid code duplication.


https://reviews.llvm.org/D46049



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

[PATCH] D46049: [OpenCL] Add constant address space to __func__ in AST

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/AST/Expr.cpp:870
+  if (C.getLangOpts().OpenCL && Ty.getAddressSpace() == LangAS::Default)
+Ty = C.getAddrSpaceQualType(Ty, LangAS::opencl_constant);
+

As `Ty` is passed by value, shouldn't we accept only data located in constant 
address space?



Comment at: lib/Sema/SemaExpr.cpp:3057
+ResTy = Context.getAddrSpaceQualType(ResTy, LangAS::opencl_constant);
   ResTy = Context.getConstantArrayType(ResTy, LengthI, ArrayType::Normal,
/*IndexTypeQuals*/ 0);

String type can only be a constant arrays.
Can we set constant address space inside this getter for OpenCL language?
Or we might want constant array in other address spaces e.g. private? 


https://reviews.llvm.org/D46049



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

[PATCH] D54858: [OpenCL] Improve diagnostics for address spaces in template instantiation

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Sema/TreeTransform.h:5276
 
+// Return type cann't be qualified with an address space.
+if (ResultType.getAddressSpace() != LangAS::Default) {

"cann't" - typo?
cann't - > can't or cannot.



Comment at: test/CodeGenOpenCLCXX/template-address-spaces.cl:28
   sintptrgl.foo();
   //sintgl.foo();
 }

I think it should comment should also be removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54858/new/

https://reviews.llvm.org/D54858



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

[PATCH] D59646: [PR40778][PR41157][OpenCL] Prevent implicit initialization of local address space objects

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:11648-11651
+// In OpenCL we don't allow to initialize objects in local address space.
+if (getLangOpts().OpenCL &&
+Var->getType().getAddressSpace() == LangAS::opencl_local)
+  return;

Shouldn't we invalidate Var declaration?



Comment at: test/CodeGenOpenCLCXX/addrspace-of-this.cl:154
 
-// Test the address space of 'this' when invoking a constructor for an object 
in non-default address space
-// EXPL: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast 
(%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+// Test that we don't initialize an onbject in local address space
+// EXPL-NOT: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast 
(%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))

onbject  -> object



Comment at: test/CodeGenOpenCLCXX/local_addrspace_init.cl:12
+  __local int i;
+  __local C ii;
+  //FIXME: In OpenCL C we don't accept initializers for local address space 
variables.

I guess this declaration should be disallowed for non-POD types. Can we add a 
check for that to some test/Sema* test?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59646/new/

https://reviews.llvm.org/D59646



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

[PATCH] D59646: [PR40778][PR41157][OpenCL] Prevent implicit initialization of local address space objects

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: test/CodeGenOpenCLCXX/local_addrspace_init.cl:12
+  __local int i;
+  __local C ii;
+  //FIXME: In OpenCL C we don't accept initializers for local address space 
variables.

Anastasia wrote:
> bader wrote:
> > I guess this declaration should be disallowed for non-POD types. Can we add 
> > a check for that to some test/Sema* test?
> What should be disallowed specifically? Declaring non-POD types in the local 
> address space?
Something like 

```
class C {
  int i = 0;
};

kernel void test() {
  __local C c; // error
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59646/new/

https://reviews.llvm.org/D59646



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

[PATCH] D57768: [SYCL] Add clang front-end option to enable SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 187785.
bader added a comment.

Add check that SYCL specific macro is not enabled by default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57768/new/

https://reviews.llvm.org/D57768

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/sycl-macro.cpp


Index: clang/test/Preprocessor/sycl-macro.cpp
===
--- /dev/null
+++ clang/test/Preprocessor/sycl-macro.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 %s -E -dM | FileCheck %s
+// RUN: %clang_cc1 %s -fsycl-is-device -E -dM | FileCheck 
--check-prefix=CHECK-SYCL %s
+
+// CHECK-NOT:#define __SYCL_DEVICE_ONLY__ 1
+// CHECK-SYCL:#define __SYCL_DEVICE_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1057,6 +1057,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) 
\
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2879,6 +2879,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -836,8 +836,14 @@
 def fopenmp_host_ir_file_path : Separate<["-"], "fopenmp-host-ir-file-path">,
   HelpText<"Path to the IR file produced by the frontend for the host.">;
 
-} // let Flags = [CC1Option]
+//===--===//
+// SYCL Options
+//===--===//
 
+def fsycl_is_device : Flag<["-"], "fsycl-is-device">,
+  HelpText<"Generate code for SYCL device.">;
+
+} // let Flags = [CC1Option]
 
 
//===--===//
 // cc1as-only Options
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -217,6 +217,8 @@
 LANGOPT(CUDADeviceApproxTranscendentals, 1, 0, "using approximate 
transcendental functions")
 LANGOPT(GPURelocatableDeviceCode, 1, 0, "generate relocatable device code")
 
+LANGOPT(SYCLIsDevice  , 1, 0, "Generate code for SYCL device")
+
 LANGOPT(SizedDeallocation , 1, 0, "sized deallocation")
 LANGOPT(AlignedAllocation , 1, 0, "aligned allocation")
 LANGOPT(AlignedAllocationUnavailable, 1, 0, "aligned allocation functions are 
unavailable")


Index: clang/test/Preprocessor/sycl-macro.cpp
===
--- /dev/null
+++ clang/test/Preprocessor/sycl-macro.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 %s -E -dM | FileCheck %s
+// RUN: %clang_cc1 %s -fsycl-is-device -E -dM | FileCheck --check-prefix=CHECK-SYCL %s
+
+// CHECK-NOT:#define __SYCL_DEVICE_ONLY__ 1
+// CHECK-SYCL:#define __SYCL_DEVICE_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1057,6 +1057,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2879,6 +2879,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = 

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader updated this revision to Diff 187751.
bader added a comment.

Applied comments from @ABataev.

- Split changes into two patches. This part contains front-end option enabling 
device specific macro. Changes adding driver option will be sent in a separate 
patch.
- Added LIT test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57768/new/

https://reviews.llvm.org/D57768

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/sycl-macro.cpp


Index: clang/test/Preprocessor/sycl-macro.cpp
===
--- /dev/null
+++ clang/test/Preprocessor/sycl-macro.cpp
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 %s -fsycl-is-device -E -dM | FileCheck %s
+
+// CHECK:#define __SYCL_DEVICE_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1057,6 +1057,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) 
\
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2879,6 +2879,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -836,8 +836,14 @@
 def fopenmp_host_ir_file_path : Separate<["-"], "fopenmp-host-ir-file-path">,
   HelpText<"Path to the IR file produced by the frontend for the host.">;
 
-} // let Flags = [CC1Option]
+//===--===//
+// SYCL Options
+//===--===//
 
+def fsycl_is_device : Flag<["-"], "fsycl-is-device">,
+  HelpText<"Generate code for SYCL device.">;
+
+} // let Flags = [CC1Option]
 
 
//===--===//
 // cc1as-only Options
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -217,6 +217,8 @@
 LANGOPT(CUDADeviceApproxTranscendentals, 1, 0, "using approximate 
transcendental functions")
 LANGOPT(GPURelocatableDeviceCode, 1, 0, "generate relocatable device code")
 
+LANGOPT(SYCLIsDevice  , 1, 0, "Generate code for SYCL device")
+
 LANGOPT(SizedDeallocation , 1, 0, "sized deallocation")
 LANGOPT(AlignedAllocation , 1, 0, "aligned allocation")
 LANGOPT(AlignedAllocationUnavailable, 1, 0, "aligned allocation functions are 
unavailable")


Index: clang/test/Preprocessor/sycl-macro.cpp
===
--- /dev/null
+++ clang/test/Preprocessor/sycl-macro.cpp
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 %s -fsycl-is-device -E -dM | FileCheck %s
+
+// CHECK:#define __SYCL_DEVICE_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1057,6 +1057,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2879,6 +2879,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX 

[PATCH] D57768: [SYCL] Add clang front-end option to enable SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader closed this revision.
bader added a comment.

Committed: https://reviews.llvm.org/rL354773


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57768/new/

https://reviews.llvm.org/D57768



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

[PATCH] D59219: [PR41007][OpenCL] Allow printf and toolchain reserved variadic functions in C++

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

> May be test/Driver/include-default-header.cl would make more sense to show 
> the intent?

test/Headers/opencl-c-header.cl sounds like good candidate.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59219/new/

https://reviews.llvm.org/D59219



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

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

I see seven OpenCL C tests using -finclude-default-header option and AFAIK, 
only test/Driver/include-default-header.cl doesn't parse it. Could you also 
update OpenCL C tests?
I think clang/test/Headers/opencl-c-header.cl and 
test/Driver/include-default-header.cl fully cover this feature. 
All other tests seems to use this option to simplify the test, but with 
additional cost on parsing this header.

grep -r include-default-header clang/test/

clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -emit-llvm -o - -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD 
%s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -emit-llvm -o - -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD 
%s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 
--check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 
--check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir64-unknown-unknown -emit-llvm -o - -cl-std=CL1.2 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 
| FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple amdgcn--amdhsa 
-O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | 
FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir64-unknown-unknown -emit-llvm -o - -cl-std=CL1.2 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 
| FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple amdgcn--amdhsa 
-O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | 
FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Driver/include-default-header.cl:// RUN: %clang -save-temps -x cl 
-Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -emit-llvm -S -### %s
clang/test/Driver/include-default-header.cl:// CHECK-NOT: 
finclude-default-header
clang/test/Driver/include-default-header.cl:// Make sure we don't pass 
-finclude-default-header to any commands other than the driver.
clang/test/SemaOpenCL/as_type.cl:// RUN: %clang_cc1 %s -emit-llvm -triple 
spir-unknown-unknown -finclude-default-header -o - -verify -fsyntax-only
clang/test/SemaOpenCL/printf-format-string-warnings.cl:// RUN: %clang_cc1 %s 
-verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
clang/test/SemaOpenCL/extensions.cl:// Test with -finclude-default-header, 
which includes opencl-c.h. opencl-c.h
clang/test/SemaOpenCL/extensions.cl:// RUN: %clang_cc1 %s -triple 
amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 
-finclude-default-header
clang/test/SemaOpenCL/extensions.cl:// RUN: %clang_cc1 %s -triple 
spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++ 
-finclude-default-header
clang/test/CodeGenOpenCL/builtins.cl:// RUN: %clang_cc1 %s 
-finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple 
"spir-unknown-unknown" | FileCheck %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple spir-unknown-unknown -o - | 
FileCheck --check-prefix=SZ32 %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple spir64-unknown-unknown -o - | 
FileCheck --check-prefix=SZ64 --check-prefix=SZ64ONLY %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple amdgcn -o - | FileCheck 
--check-prefix=SZ64 --check-prefix=AMDGCN %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple amdgcn---opencl -o - | 
FileCheck --check-prefix=SZ64 --check-prefix=AMDGCN %s




Comment at: test/Driver/include-default-header.cl:2
+// RUN: %clang -save-temps -x cl -Xclang 

[PATCH] D59492: [OpenCL] Allow variadic macros as Clang feature

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59492/new/

https://reviews.llvm.org/D59492



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

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

OpenCL C++ part looks good. Thanks!




Comment at: test/Headers/opencl-c-header.cl:57-65
 char f(char x) {
-#if __OPENCL_C_VERSION__ != CL_VERSION_2_0
+#if !defined(__OPENCL_CPP_VERSION__) && (__OPENCL_C_VERSION__ != 
CL_VERSION_2_0)
   return convert_char_rte(x);
 
 #else //__OPENCL_C_VERSION__
   ndrange_t t;
   return ctz(x);

This test looks strange.
It checks that convert_char_rte is compiled if OpenCL version is not 2.0, but 
AFAIK it's not deprecated in OpenCL C 2.0, so I don't know why test is written 
in this way (i.e. ifdef-else-endif).

I think checks should look like this:
```
char f(char x) {
// Check check OpenCL C 2.0 and OpenCL C++ functionality 
#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)
  ndrange_t t;
  x = ctz(x);
#endif
  return convert_char_rte(x);
}
```

Probably it's better to fix separately.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59486/new/

https://reviews.llvm.org/D59486



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

[PATCH] D59544: [OpenCL] Minor improvements in default header testing

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59544/new/

https://reviews.llvm.org/D59544



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

[PATCH] D59219: [PR41007][OpenCL] Allow printf and toolchain reserved variadic functions in C++

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: cfe/trunk/test/SemaOpenCL/extensions.cl:31
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++ -finclude-default-header
 

Shoudn't we move this test to test/SemaOpenCLCXX?
Does `-finclude-default-header` include opencl-c.h? I think it's an overkill to 
test that OpenCL C++ allows printf. Ideally we should minimize number of times 
we parsing 11500+ lines header in LIT tests.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59219/new/

https://reviews.llvm.org/D59219



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

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D57768#1386924 , @ABataev wrote:

> This definitely requires a test.


@ABataev, I tried to find some tests on similar `-fcuda-is-device` and 
`-fopenmp-is-device` options, but I wasn't able to find a dedicated test. Could 
you suggest some examples testing similar functionality, please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57768/new/

https://reviews.llvm.org/D57768



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

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D57768#1386754 , @Naghasan wrote:

> LGTM
>
> Side note: might be good to also involve @Anastasia, as some of the future 
> patches will overlap with OpenCL.


Sure. I'll add @Anastasia as a reviewer to the relevant patches.

Thanks,
Alexey


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57768/new/

https://reviews.llvm.org/D57768



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

[PATCH] D58277: [OpenCL] Change type of block pointer for OpenCL

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354337: [OpenCL] Change type of block pointer for OpenCL 
(authored by bader, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58277?vs=186988=187363#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58277/new/

https://reviews.llvm.org/D58277

Files:
  cfe/trunk/lib/CodeGen/CodeGenTypes.cpp
  cfe/trunk/test/CodeGenOpenCL/blocks.cl
  cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl

Index: cfe/trunk/lib/CodeGen/CodeGenTypes.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenTypes.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenTypes.cpp
@@ -635,7 +635,9 @@
 
   case Type::BlockPointer: {
 const QualType FTy = cast(Ty)->getPointeeType();
-llvm::Type *PointeeType = ConvertTypeForMem(FTy);
+llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
+  ? CGM.getGenericBlockLiteralType()
+  : ConvertTypeForMem(FTy);
 unsigned AS = Context.getTargetAddressSpace(FTy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
Index: cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
===
--- cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
+++ cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
@@ -11,7 +11,7 @@
 
 // For a block global variable, first emit the block literal as a global variable, then emit the block variable itself.
 // COMMON: [[BL_GLOBAL:@__block_literal_global[^ ]*]] = internal addrspace(1) constant { i32, i32, i8 addrspace(4)* } { i32 {{[0-9]+}}, i32 {{[0-9]+}}, i8 addrspace(4)* addrspacecast (i8* bitcast (void (i8 addrspace(4)*, i8 addrspace(3)*)* [[INV_G:@[^ ]+]] to i8*) to i8 addrspace(4)*) }
-// COMMON: @block_G =  addrspace(1) constant void (i8 addrspace(3)*) addrspace(4)* addrspacecast (void (i8 addrspace(3)*) addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BL_GLOBAL]] to void (i8 addrspace(3)*) addrspace(1)*) to void (i8 addrspace(3)*) addrspace(4)*)
+// COMMON: @block_G = addrspace(1) constant %struct.__opencl_block_literal_generic addrspace(4)* addrspacecast (%struct.__opencl_block_literal_generic addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BL_GLOBAL]] to %struct.__opencl_block_literal_generic addrspace(1)*) to %struct.__opencl_block_literal_generic addrspace(4)*)
 
 // For anonymous blocks without captures, emit block literals as global variable.
 // COMMON: [[BLG1:@__block_literal_global[^ ]*]] = internal addrspace(1) constant { i32, i32, i8 addrspace(4)* } { i32 {{[0-9]+}}, i32 {{[0-9]+}}, i8 addrspace(4)* addrspacecast (i8* bitcast (void (i8 addrspace(4)*, i8 addrspace(3)*)* {{@[^ ]+}} to i8*) to i8 addrspace(4)*) }
@@ -77,9 +77,9 @@
   // COMMON: [[DEF_Q:%[0-9]+]] = load %opencl.queue_t{{.*}}*, %opencl.queue_t{{.*}}** %default_queue
   // COMMON: [[FLAGS:%[0-9]+]] = load i32, i32* %flags
   // COMMON: store i8 addrspace(4)* addrspacecast (i8* bitcast (void (i8 addrspace(4)*)* [[INVL1:@__device_side_enqueue_block_invoke[^ ]*]] to i8*) to i8 addrspace(4)*), i8 addrspace(4)** %block.invoke
-  // B32: [[BL:%[0-9]+]] = bitcast <{ i32, i32, i8 addrspace(4)*, i32 addrspace(1)*, i32, i32 addrspace(1)* }>* %block to void ()*
-  // B64: [[BL:%[0-9]+]] = bitcast <{ i32, i32, i8 addrspace(4)*, i32 addrspace(1)*, i32 addrspace(1)*, i32 }>* %block to void ()*
-  // COMMON: [[BL_I8:%[0-9]+]] = addrspacecast void ()* [[BL]] to i8 addrspace(4)*
+  // B32: [[BL:%[0-9]+]] = bitcast <{ i32, i32, i8 addrspace(4)*, i32 addrspace(1)*, i32, i32 addrspace(1)* }>* %block to %struct.__opencl_block_literal_generic*
+  // B64: [[BL:%[0-9]+]] = bitcast <{ i32, i32, i8 addrspace(4)*, i32 addrspace(1)*, i32 addrspace(1)*, i32 }>* %block to %struct.__opencl_block_literal_generic*
+  // COMMON: [[BL_I8:%[0-9]+]] = addrspacecast %struct.__opencl_block_literal_generic* [[BL]] to i8 addrspace(4)*
   // COMMON-LABEL: call i32 @__enqueue_kernel_basic(
   // COMMON-SAME: %opencl.queue_t{{.*}}* [[DEF_Q]], i32 [[FLAGS]], %struct.ndrange_t* byval [[NDR]]{{([0-9]+)?}},
   // COMMON-SAME: i8 addrspace(4)* addrspacecast (i8* bitcast ({{.*}} [[INVLK1:[^ ]+_kernel]] to i8*) to i8 addrspace(4)*),
@@ -95,8 +95,8 @@
   // COMMON: [[WAIT_EVNT:%[0-9]+]] = addrspacecast %opencl.clk_event_t{{.*}}** %event_wait_list to %opencl.clk_event_t{{.*}}* addrspace(4)*
   // COMMON: [[EVNT:%[0-9]+]] = addrspacecast %opencl.clk_event_t{{.*}}** %clk_event to %opencl.clk_event_t{{.*}}* addrspace(4)*
   // COMMON: store i8 addrspace(4)* addrspacecast (i8* bitcast (void (i8 addrspace(4)*)* [[INVL2:@__device_side_enqueue_block_invoke[^ ]*]] to i8*) to i8 addrspace(4)*), i8 addrspace(4)** %block.invoke
-  // COMMON: [[BL:%[0-9]+]] = bitcast <{ i32, i32, i8 addrspace(4)*, 

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D57768#1386941 , @ABataev wrote:

> In D57768#1386933 , @bader wrote:
>
> > In D57768#1386924 , @ABataev wrote:
> >
> > > This definitely requires a test.
> >
> >
> > @ABataev, I tried to find some tests on similar `-fcuda-is-device` and 
> > `-fopenmp-is-device` options, but I wasn't able to find a dedicated test. 
> > Could you suggest some examples testing similar functionality, please?
>
>
> There are several similar tests:
>  OpenMP/driver.c, Driver/openmp-offload.c, Driver/openmp-offload-gpu.c. There 
> is no absolutely the same test for OpenMP, since OpenMP has mo similar req 
> for the offloading.


@ABataev thanks for the pointers. The uploaded patch adds two options:

- fsycl-is-device (front-end option)
- sycl-device-only (driver option)

The driver tests you mention validate driver logic enabled by new options, 
which is not part of this test and I was going to add it later.
I can split the patch and remove new driver option and leave only front-end 
option.
Another option is to add driver logic that invokes the front-end compiler in 
"device only" mode.
Which option do you prefer?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57768/new/

https://reviews.llvm.org/D57768



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

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
bader added reviewers: keryell, Naghasan.
Herald added subscribers: cfe-commits, ebevhan.
Herald added a project: clang.

-fsycl-is-device enables compilation of the device part of SYCL source
file.

Patch by Mariya Podchishchaeva 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57768

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/InitPreprocessor.cpp


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1056,6 +1056,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2875,6 +2875,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3150,6 +3150,9 @@
 defm underscoring : BooleanFFlag<"underscoring">, Group;
 defm whole_file : BooleanFFlag<"whole-file">, Group;
 
+// SYCL options
+def sycl_device_only : Flag<["--"], "sycl-device-only">,
+  HelpText<"Compile SYCL code for device only">;
 
 include "CC1Options.td"
 
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -836,8 +836,14 @@
 def fopenmp_host_ir_file_path : Separate<["-"], "fopenmp-host-ir-file-path">,
   HelpText<"Path to the IR file produced by the frontend for the host.">;
 
-} // let Flags = [CC1Option]
+//===--===//
+// SYCL Options
+//===--===//
 
+def fsycl_is_device : Flag<["-"], "fsycl-is-device">,
+  HelpText<"Generate code for SYCL device.">;
+
+} // let Flags = [CC1Option]
 
 
//===--===//
 // cc1as-only Options
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -215,6 +215,8 @@
 LANGOPT(CUDADeviceApproxTranscendentals, 1, 0, "using approximate 
transcendental functions")
 LANGOPT(GPURelocatableDeviceCode, 1, 0, "generate relocatable device code")
 
+LANGOPT(SYCLIsDevice  , 1, 0, "Generate code for SYCL device")
+
 LANGOPT(SizedDeallocation , 1, 0, "sized deallocation")
 LANGOPT(AlignedAllocation , 1, 0, "aligned allocation")
 LANGOPT(AlignedAllocationUnavailable, 1, 0, "aligned allocation functions are 
unavailable")


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1056,6 +1056,12 @@
 Builder.defineMacro("__CLANG_CUDA_APPROX_TRANSCENDENTALS__");
   }
 
+  // Define indicating that the source file is being compiled with a SYCL
+  // device compiler which doesn't produce host binary.
+  if (LangOpts.SYCLIsDevice) {
+Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
+  }
+
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
 #define OPENCLEXT(Ext) \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2875,6 +2875,8 @@
   << Opts.OMPHostIRFile;
   }
 
+  Opts.SYCLIsDevice = Args.hasArg(options::OPT_fsycl_is_device);
+
   // Set CUDA mode for OpenMP target NVPTX if specified in options
   Opts.OpenMPCUDAMode = Opts.OpenMPIsDevice && T.isNVPTX() &&
 Args.hasArg(options::OPT_fopenmp_cuda_mode);
Index: clang/include/clang/Driver/Options.td
===
--- 

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

To give more context to the question, I'd like to clarify the use case of new 
attributes.

As @Fznamznon already mentioned in previous comments SYCL is not supposed to 
expose any non-standard extensions to a user.

Here is code example of the SYCL program, which demonstrate the need for these 
attributes:

  C++
  int foo(int x) { return ++x; }
  int bar(int x) { throw std::exception("CPU code only!"); }
  …
  using namespace cl::sycl;
  queue Q;
  buffer a(range<1>{1024});
  Q.submit([&](handler& cgh) {
auto A = a.get_access(cgh);
cgh.parallel_for(range<1>{1024}, [=](id<1> index) {
  A[index] = index[0] * 2 + index[1] + foo(42);
});
  }
  ...

SYCL compiler need to compile lambda function passed to 
`cl::sycl::handler::parallel_for` method and function `foo` called from this 
lambda function.

NOTE: compiler must ignore `bar` function when we "device" part of the single 
source code.

Our current approach is to add an attribute, which SYCL runtime will use to 
mark code passed to `cl::sycl::handler::parallel_for` as "kernel functions". 
Obviously runtime library can't mark `foo` as "device" code - this is a 
compiler job: to traverse all symbols accessible from kernel functions and add 
them to the "device part" of the code.

Here is a link to the code in the SYCL runtime using `sycl_kernel` attribute: 
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L267

I'm quite sure something similar should happen for other "single source" 
programming models like OpenMP/CUDA, except these attributes are exposed to the 
user and there is a specific requirement on attributes/pragma/keyword names.

What we are looking for is whether we should add SYCL specific code or we can 
come up with something more generic to avoid logic/code duplication with 
already existing functionality.

BTW: Mariya, I think we might need to use `sycl_device` attribute to mark 
functions, which are called from the different translation units, i.e. compiler 
can't identify it w/o user's help.
SYCL specification proposes to use special macro as "device function marker", 
but I guess we can have additional "spellings" in the clang.

NOTE2: @Anastasia, https://reviews.llvm.org/D60454 makes impossible to replace 
`sycl_kernel` attribute with `__kernel` attribute. I mean we still can enable 
it for SYCL extension, but we will need SYCL specific customization in this 
case as we apply `kernel` attribute to a template function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.

LGTM. One minor comment.




Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:3
+// Now pretend that we're compiling a C++ file. There should be warnings.
+// RUN: %clang_cc1 -DEXPECT_WARNINGS -fsyntax-only -verify -x c++ %s
+

No need to pass "EXPECT_WARNINGS" define.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



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