[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal updated this revision to Diff 502855.
Ayal added a comment.

It's hard for getOpenCLKernelParameterType() to detect and diagnose invalid 
pointer cases w/o context (of an enclosing struct or not), but it's easy to 
detect valid pointer cases for v2.0+ and return ValidKernelParam.

Rebased.


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

https://reviews.llvm.org/D143849

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCL/invalid-kernel-parameters.cl


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,ocl12 %s -triple 
spir-unknown-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown 
-cl-std=CL2.0
 
 kernel void half_arg(half x) { } // expected-error{{declaring function 
parameter of type '__private half' is not allowed; did you forget * ?}}
@@ -104,6 +104,13 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
 
+typedef struct FooGlobal // ocl12-note{{within field of type 'FooGlobal' 
declared here}}
+{
+  global int* ptrField; // ocl12-note{{field of illegal pointer type '__global 
int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // 
ocl12-error{{struct kernel parameters may not contain pointers}}
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' 
declared here}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type 
'__{{private|generic}} int *' declared here}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9273,6 +9273,12 @@
   ParamKind == InvalidKernelParam)
 return ParamKind;
 
+  // OpenCL v3.0 s6.11.a:
+  // A restriction to pass pointers to pointers only applies to OpenCL C
+  // v1.2 or below.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
+return ValidKernelParam;
+
   return PtrPtrKernelParam;
 }
 
@@ -9300,6 +9306,11 @@
   return InvalidKernelParam;
 }
 
+// OpenCL v1.2 s6.9.p:
+// A restriction to pass pointers only applies to OpenCL C v1.2 or below.
+if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
+  return ValidKernelParam;
+
 return PtrKernelParam;
   }
 
@@ -9366,13 +9377,8 @@
 // OpenCL v3.0 s6.11.a:
 // A kernel function argument cannot be declared as a pointer to a pointer
 // type. [...] This restriction only applies to OpenCL C 1.2 or below.
-if (S.getLangOpts().getOpenCLCompatibleVersion() <= 120) {
-  S.Diag(Param->getLocation(), diag::err_opencl_ptrptr_kernel_param);
-  D.setInvalidType();
-  return;
-}
-
-ValidTypes.insert(PT.getTypePtr());
+S.Diag(Param->getLocation(), diag::err_opencl_ptrptr_kernel_param);
+D.setInvalidType();
 return;
 
   case InvalidAddrSpacePtrKernelParam:
@@ -9490,7 +9496,8 @@
   // OpenCL v1.2 s6.9.p:
   // Arguments to kernel functions that are declared to be a struct or 
union
   // do not allow OpenCL objects to be passed as elements of the struct or
-  // union.
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||
   ParamType == InvalidAddrSpacePtrKernelParam) {
 S.Diag(Param->getLocation(),


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,ocl12 %s -triple spir-unknown-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown -cl-std=CL2.0
 
 kernel void half_arg(half x) { } // expected-error{{declaring function parameter of type '__private half' is not allowed; did you forget * ?}}
@@ -104,6 +104,13 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct kernel parameters may not contain pointers}}
 
+typedef struct FooGlobal // ocl12-note{{within field of type 'FooGlobal' declared here}}
+{
+  global int* ptrField; // ocl12-note{{field of illegal pointer type '__global int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // ocl12-error{{struct kernel parameters may not contain pointers}}
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' decla

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

Anastasia wrote:
> Ayal wrote:
> > Anastasia wrote:
> > > Anastasia wrote:
> > > > Ayal wrote:
> > > > > Anastasia wrote:
> > > > > > I think it should be possible to merge this with `if` below to 
> > > > > > avoid condition duplication.
> > > > > > 
> > > > > > 
> > > > > Sure, but that trades one duplication for another, rather than 
> > > > > clearly separating the early-continue case early?
> > > > > 
> > > > > ```
> > > > >   if (ParamType == PtrKernelParam || ParamType == 
> > > > > PtrPtrKernelParam) {
> > > > > if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
> > > > >   continue;
> > > > > S.Diag(Param->getLocation(),
> > > > >diag::err_record_with_pointers_kernel_param)
> > > > >   << PT->isUnionType()
> > > > >   << PT;
> > > > >   } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
> > > > > S.Diag(Param->getLocation(),
> > > > >diag::err_record_with_pointers_kernel_param)
> > > > >   << PT->isUnionType()
> > > > >   << PT;
> > > > >   } else {
> > > > > S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) 
> > > > > << PT;
> > > > > 
> > > > > ```
> > > > I am mainly thinking in terms of maintenance if for example someone 
> > > > fixes one if and forgets another. Or if ifs will get separated by some 
> > > > other code and then it is not easy to see that the same thing is 
> > > > handled differently in OpenCL versions. 
> > > > 
> > > > Unfortunately we have a lot of those cases, I know this function has 
> > > > early exists but it is not a common style.
> > > > 
> > > > 
> > > > I was talking about something like:
> > > > 
> > > > 
> > > > ```
> > > > if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
> > > > (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
> > > >   ParamType == InvalidAddrSpacePtrKernelParam)
> > > > ```
> > > > 
> > > > It would also be ok to separate `InvalidAddrSpacePtrKernelParam` since 
> > > > it's handling different feature.
> > > Sorry I have forgotten that this part of the code is expected to handle 
> > > the diagnostics only. The decision that the kernel parameter is wrong is 
> > > done in `getOpenCLKernelParameterType`. I think you should alter the 
> > > conditions there to check for OpenCL version and avoid classifying cases 
> > > you care about as `PtrKernelParam` or `PtrPtrKernelParam`. Then here you 
> > > wouldn't need this extra if/continue block. 
> > Hmm, would that be better? This part of the code, namely 
> > `checkIsValidOpenCLKernelParameter()`, does check the validity of arguments 
> > classified by `getOpenCLKernelParameterType()` in addition to handling 
> > diagnostics. E.g., the first case above decides that arguments of 
> > pointer-to-pointer type are wrong along with proper diagnostics for OpenCL 
> > 1.x while allowing them for other OpenCL versions.
> > 
> > Struct arguments are simply classified as records by 
> > getOpenCLKernelParameterType(), whereas this part of the code traverses 
> > each struct and calls getOpenCLKernelParameterType() on each field - the 
> > latter seems  unaware if it was invoked on a struct field or not? If it is 
> > (made) aware, it could indeed return a (new kind of?) invalid type instead 
> > of pointer type for OpenCL 1.x - how would the right 
> > err_record_with_pointers_kernel_param diagnostics then be handled? If 
> > desired, such refactoring should probably be done independent of this fix?
> That's right there is a mix of everything as I think this code has deviated 
> from its original idea, but I still think it's cleaner to avoid doing the 
> same kind of checks in multiple places.
> 
> 
> Overall I find this code a bit over-engineered, this is maybe why it went off 
> track. So some refactoring would indeed be helpful. However I am not sure 
> whether it's better to continue the same route or try to simplify the design 
> by just adding separate functions for each error case that would detect that 
> the type is of a certain kind e.g a pointer or a pointer with wrong address 
> space and then also provide the diagnostic straight away . I feel this would 
> match better the rest of the diagnostic handling in clang but might result in 
> slightly more helper functions or duplication of code.
Tried to avoid doing the same checks in multiple places by having 
getOpenCLKernelParameterType() identify valid pointer cases as 
ValidKernelParam, please see above.

Does this look ok? If some (NFC) refactoring is still desired, better apply it 
after this bug fix?


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

https://reviews.llvm.org/D143849

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGeae70ccbf975: [Clang][OpenCL] Allow pointers in structs as 
kernel arguments from 2.0 (authored by Ayal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143849

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCL/invalid-kernel-parameters.cl


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,ocl12 %s -triple 
spir-unknown-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown 
-cl-std=CL2.0
 
 kernel void half_arg(half x) { } // expected-error{{declaring function 
parameter of type '__private half' is not allowed; did you forget * ?}}
@@ -104,6 +104,13 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
 
+typedef struct FooGlobal // ocl12-note{{within field of type 'FooGlobal' 
declared here}}
+{
+  global int* ptrField; // ocl12-note{{field of illegal pointer type '__global 
int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // 
ocl12-error{{struct kernel parameters may not contain pointers}}
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' 
declared here}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type 
'__{{private|generic}} int *' declared here}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9273,6 +9273,12 @@
   ParamKind == InvalidKernelParam)
 return ParamKind;
 
+  // OpenCL v3.0 s6.11.a:
+  // A restriction to pass pointers to pointers only applies to OpenCL C
+  // v1.2 or below.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
+return ValidKernelParam;
+
   return PtrPtrKernelParam;
 }
 
@@ -9300,6 +9306,11 @@
   return InvalidKernelParam;
 }
 
+// OpenCL v1.2 s6.9.p:
+// A restriction to pass pointers only applies to OpenCL C v1.2 or below.
+if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
+  return ValidKernelParam;
+
 return PtrKernelParam;
   }
 
@@ -9366,13 +9377,8 @@
 // OpenCL v3.0 s6.11.a:
 // A kernel function argument cannot be declared as a pointer to a pointer
 // type. [...] This restriction only applies to OpenCL C 1.2 or below.
-if (S.getLangOpts().getOpenCLCompatibleVersion() <= 120) {
-  S.Diag(Param->getLocation(), diag::err_opencl_ptrptr_kernel_param);
-  D.setInvalidType();
-  return;
-}
-
-ValidTypes.insert(PT.getTypePtr());
+S.Diag(Param->getLocation(), diag::err_opencl_ptrptr_kernel_param);
+D.setInvalidType();
 return;
 
   case InvalidAddrSpacePtrKernelParam:
@@ -9490,7 +9496,8 @@
   // OpenCL v1.2 s6.9.p:
   // Arguments to kernel functions that are declared to be a struct or 
union
   // do not allow OpenCL objects to be passed as elements of the struct or
-  // union.
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||
   ParamType == InvalidAddrSpacePtrKernelParam) {
 S.Diag(Param->getLocation(),


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,ocl12 %s -triple spir-unknown-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown -cl-std=CL2.0
 
 kernel void half_arg(half x) { } // expected-error{{declaring function parameter of type '__private half' is not allowed; did you forget * ?}}
@@ -104,6 +104,13 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct kernel parameters may not contain pointers}}
 
+typedef struct FooGlobal // ocl12-note{{within field of type 'FooGlobal' declared here}}
+{
+  global int* ptrField; // ocl12-note{{field of illegal pointer type '__global int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // ocl12-error{{struct kernel parameters may not contain pointers}}
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' declared

[PATCH] D49723: [OpenCL] Check for invalid kernel arguments in array types

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added a comment.
Herald added a subscriber: Naghasan.
Herald added a project: All.

In D49723#1178186 , @asavonic wrote:

> In D49723#1178127 , @Anastasia wrote:
>
>> In D49723#1174837 , @asavonic wrote:
>>
>>> In D49723#1173352 , @Anastasia 
>>> wrote:
>>>
 Btw, has this restriction been removed from CL 2.0?
>>>
>>> No, it applies for CL2.0 as well.
>>
>> It seems however the restriction on pointer to pointer was removed (see 
>> s6.9.a last item) in CL2.0.
>
> Right, and it seems that pointers in struct arguments should also be legal in 
> CL2.0.
> I'll submit another patch to remove this check for CL2.0.

This is admittedly a couple of years old by now, but wonder about that other 
intended patch - Clang still seems to consider pointers in struct arguments to 
be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d


Repository:
  rC Clang

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

https://reviews.llvm.org/D49723

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal created this revision.
Ayal added reviewers: asavonic, Anastasia, yaxunl.
Herald added subscribers: Naghasan, ldrumm.
Herald added a project: All.
Ayal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Structs that contain global or local pointers can be passed as kernel
arguments starting OpenCL v2.0 which introduces shared virtual memory.

Trying to revive a patch considered in D49723 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143849

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCL/invalid-kernel-parameters.cl


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -87,6 +87,7 @@
 
 
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
 typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' 
declared here}}
 {
   // TODO: Clean up needed - we don't really need to check for image, event, 
etc
@@ -96,6 +97,7 @@
 } FooImage2D;
 
 kernel void image_in_struct_arg(FooImage2D arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
+#endif
 
 typedef struct Foo // expected-note{{within field of type 'Foo' declared here}}
 {
@@ -104,6 +106,15 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
+typedef struct FooGlobal // expected-note{{within field of type 'FooGlobal' 
declared here}}
+{
+  global int* ptrField; // expected-note{{field of illegal pointer type 
'__global int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // 
expected-error{{struct kernel parameters may not contain pointers}}
+#endif
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' 
declared here}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type 
'__{{private|generic}} int *' declared here}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9489,7 +9489,12 @@
   // OpenCL v1.2 s6.9.p:
   // Arguments to kernel functions that are declared to be a struct or 
union
   // do not allow OpenCL objects to be passed as elements of the struct or
-  // union.
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))
+continue;
+
   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||
   ParamType == InvalidAddrSpacePtrKernelParam) {
 S.Diag(Param->getLocation(),


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -87,6 +87,7 @@
 
 
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
 typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' declared here}}
 {
   // TODO: Clean up needed - we don't really need to check for image, event, etc
@@ -96,6 +97,7 @@
 } FooImage2D;
 
 kernel void image_in_struct_arg(FooImage2D arg) { } // expected-error{{struct kernel parameters may not contain pointers}}
+#endif
 
 typedef struct Foo // expected-note{{within field of type 'Foo' declared here}}
 {
@@ -104,6 +106,15 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct kernel parameters may not contain pointers}}
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
+typedef struct FooGlobal // expected-note{{within field of type 'FooGlobal' declared here}}
+{
+  global int* ptrField; // expected-note{{field of illegal pointer type '__global int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // expected-error{{struct kernel parameters may not contain pointers}}
+#endif
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' declared here}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type '__{{private|generic}} int *' declared here}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9489,7 +9489,12 @@
   // OpenCL v1.2 s6.9.p:
   // Arguments to kernel functions that are declared to be a struct or union
   // do not allow OpenCL objects to be passed as elements of the struct or
-  // union.
+  // union. This restriction was lifted in OpenCL v2.0 with the introduction
+  // of SVM.
+  if (S.ge

[PATCH] D49723: [OpenCL] Check for invalid kernel arguments in array types

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added a comment.

In D49723#4116783 , @asavonic wrote:

> In D49723#4116435 , @Ayal wrote:
>
>> This is admittedly a couple of years old by now, but wonder about that other 
>> intended patch - Clang still seems to consider pointers in struct arguments 
>> to be illegal in CL2.0 (and CL3.0) - please see 
>> https://godbolt.org/z/E87z66h1d
>
> Yeah, the patch got lost somewhere, I'm sorry...

Sure, trying to resurrect it in a comment above, and in D143849 
.

> TBH, I don't know why pointers in structs or arrays were disallowed in the 
> first place. Even OpenCL 1.2 does not say it explicitly, although there is a 
> bit suspicious point in s6.9.p:
>
>   Arguments to kernel functions that are declared to be a struct or union do 
> not allow
>   OpenCL objects to be passed as elements of the struct or union
>
> It does not say "pointers", just "OpenCL objects". Sounds more like events or 
> images to me, not pointers.

Yeah, there's room to improve clarity of the spec. Passing pointers indirectly 
requires them to point to same/share address space, as provided by SVM starting 
in OpenCL2.0.




Comment at: lib/Sema/SemaDecl.cpp:8218
   // do not allow OpenCL objects to be passed as elements of the struct or
   // union.
   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||

Suggest to (rebase and) update this as follows, based on
https://registry.khronos.org/OpenCL/sdk/2.0/docs/man/xhtml/restrictions.html
```
// union. This restriction was lifted in OpenCL v2.0 with the introduction
// of SVM.
if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
(ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))
  continue;
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D49723

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added a comment.

In D143849#4121348 , @Anastasia wrote:

> I feel that originally pointers were disallowed because they create the same 
> issue as `size_t` and etc as their size is implementation depended but the 
> same logic applies to images and other types that are even more 
> implementation depended. Overall this bit of the spec is very inconsistent so 
> I don't mind if we change the behavior to be whatever we find more helpful. 
> However I would encourage to submit an issue to OpenCL-Docs to point out this 
> inconsistency.

Yeah, pointer sizes need to match, pointers should  be passed to 
clSetKernelExecInfo, and SVM should be employed, which the spec should clarify. 
In any case the quoted restriction "s6.9.p" of 
https://registry.khronos.org/OpenCL/sdk/1.2/docs/man/xhtml/restrictions.html 
justifying the SemA enforcement is clearly absent in 
https://registry.khronos.org/OpenCL/sdk/2.0/docs/man/xhtml/restrictions.html.




Comment at: clang/lib/Sema/SemaDecl.cpp:9493
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&

Anastasia wrote:
> To be honest I feel like it was a bug fix? Do you happen to have any record 
> of the fix?
Not sure what was the alleged bug nor its fix, but in any case I have no record 
thereof..



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

Anastasia wrote:
> I think it should be possible to merge this with `if` below to avoid 
> condition duplication.
> 
> 
Sure, but that trades one duplication for another, rather than clearly 
separating the early-continue case early?

```
  if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) {
if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
  continue;
S.Diag(Param->getLocation(),
   diag::err_record_with_pointers_kernel_param)
  << PT->isUnionType()
  << PT;
  } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
S.Diag(Param->getLocation(),
   diag::err_record_with_pointers_kernel_param)
  << PT->isUnionType()
  << PT;
  } else {
S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143849

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal updated this revision to Diff 497128.
Ayal added a comment.

Updated version merges the `if`'s and checks tests for both 1.2 and 2.0.


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

https://reviews.llvm.org/D143849

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCL/invalid-kernel-parameters.cl


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -87,15 +87,20 @@
 
 
 
-typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' 
declared here}}
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
+// expected-note@+4{{within field of type 'FooImage2D' declared here}}
+// expected-note@+8{{field of illegal type '__read_only image2d_t' declared 
here}}
+// expected-error@+10{{struct kernel parameters may not contain pointers}}
+#endif
+typedef struct FooImage2D
 {
   // TODO: Clean up needed - we don't really need to check for image, event, 
etc
   // as a note here any longer.
   // They are diagnosed as an error for all struct fields (OpenCL v1.2 
s6.9b,r).
-  image2d_t imageField; // expected-note{{field of illegal type '__read_only 
image2d_t' declared here}} expected-error{{the '__read_only image2d_t' type 
cannot be used to declare a structure or union field}}
+  image2d_t imageField; // expected-error{{the '__read_only image2d_t' type 
cannot be used to declare a structure or union field}}
 } FooImage2D;
 
-kernel void image_in_struct_arg(FooImage2D arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
+kernel void image_in_struct_arg(FooImage2D arg) { }
 
 typedef struct Foo // expected-note{{within field of type 'Foo' declared here}}
 {
@@ -104,6 +109,18 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
+// expected-note@+4{{within field of type 'FooGlobal' declared here}}
+// expected-note@+5{{field of illegal pointer type '__global int *' declared 
here}}
+// expected-error@+7{{struct kernel parameters may not contain pointers}}
+#endif
+typedef struct FooGlobal 
+{
+  global int* ptrField;
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { }
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' 
declared here}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type 
'__{{private|generic}} int *' declared here}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9489,13 +9489,18 @@
   // OpenCL v1.2 s6.9.p:
   // Arguments to kernel functions that are declared to be a struct or 
union
   // do not allow OpenCL objects to be passed as elements of the struct or
-  // union.
-  if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||
-  ParamType == InvalidAddrSpacePtrKernelParam) {
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
+  if (ParamType == InvalidAddrSpacePtrKernelParam ||
+  ((ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) &&
+   S.getLangOpts().getOpenCLCompatibleVersion() <= 120)) {
 S.Diag(Param->getLocation(),
diag::err_record_with_pointers_kernel_param)
   << PT->isUnionType()
   << PT;
+  } else if (ParamType == PtrKernelParam || ParamType == 
PtrPtrKernelParam) {
+// and OpenCL version is at-least 2.0, so these types are allowed. 
+continue;
   } else {
 S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
   }


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -87,15 +87,20 @@
 
 
 
-typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' declared here}}
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
+// expected-note@+4{{within field of type 'FooImage2D' declared here}}
+// expected-note@+8{{field of illegal type '__read_only image2d_t' declared here}}
+// expected-error@+10{{struct kernel parameters may not contain pointers}}
+#endif
+typedef struct FooImage2D
 {
   // TODO: Clean up needed - we don't really need to check for image, event, etc
   // as a note here any longer.
   // They are diagnosed as an error for all struct fields (OpenCL v1.2 s6.9b,r).
-  image2d_t imageField; // expected-note{{field of illegal type '__read_only image2d_t' declared here}} expected-error{{the '__read_only image2d_t' type cannot be used to declare a structure or union field}}
+  image2d_t imageField; // expected-error{

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added inline comments.



Comment at: clang/test/SemaOpenCL/invalid-kernel-parameters.cl:90
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
 typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' 
declared here}}

yaxunl wrote:
> we should not limit the tests to CL1.2. We should test them with 2.0 to make 
> sure there is no diagnostics.
> 
> To differentiate between 1.2 and 2.0 you can use -verify=ocl12 and 
> -verify=ocl20.
> 
> same as below
Using -verify=ocl12 and -verify=ocl20 should probably be done separately as an 
NFC patch as it involves many changes IIUC.

The expect's were placed under #ifdef instead (and only them), following e.g. 
existing lines 8-12.


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

https://reviews.llvm.org/D143849

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal updated this revision to Diff 497317.
Ayal added a comment.

Use `-verify=expected,ocl12` instead of #ifdef'ing the test to check 
diagnostics emitted for 1.2 but not emitted for 2.0.


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

https://reviews.llvm.org/D143849

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCL/invalid-kernel-parameters.cl


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,ocl12 %s -triple 
spir-unknown-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown 
-cl-std=CL2.0
 
 kernel void half_arg(half x) { } // expected-error{{declaring function 
parameter of type '__private half' is not allowed; did you forget * ?}}
@@ -87,15 +87,16 @@
 
 
 
-typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' 
declared here}}
+typedef struct FooImage2D // ocl12-note{{within field of type 'FooImage2D' 
declared here}}
 {
   // TODO: Clean up needed - we don't really need to check for image, event, 
etc
   // as a note here any longer.
   // They are diagnosed as an error for all struct fields (OpenCL v1.2 
s6.9b,r).
-  image2d_t imageField; // expected-note{{field of illegal type '__read_only 
image2d_t' declared here}} expected-error{{the '__read_only image2d_t' type 
cannot be used to declare a structure or union field}}
+  image2d_t imageField; // ocl12-note{{field of illegal type '__read_only 
image2d_t' declared here}}
+// expected-error@-1{{the '__read_only image2d_t' type 
cannot be used to declare a structure or union field}}
 } FooImage2D;
 
-kernel void image_in_struct_arg(FooImage2D arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
+kernel void image_in_struct_arg(FooImage2D arg) { } // ocl12-error{{struct 
kernel parameters may not contain pointers}}
 
 typedef struct Foo // expected-note{{within field of type 'Foo' declared here}}
 {
@@ -104,6 +105,13 @@
 
 kernel void pointer_in_struct_arg(Foo arg) { } // expected-error{{struct 
kernel parameters may not contain pointers}}
 
+typedef struct FooGlobal // ocl12-note{{within field of type 'FooGlobal' 
declared here}}
+{
+  global int* ptrField; // ocl12-note{{field of illegal pointer type '__global 
int *' declared here}}
+} FooGlobal;
+
+kernel void global_pointer_in_struct_arg(FooGlobal arg) { } // 
ocl12-error{{struct kernel parameters may not contain pointers}}
+
 typedef union FooUnion // expected-note{{within field of type 'FooUnion' 
declared here}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type 
'__{{private|generic}} int *' declared here}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9489,13 +9489,18 @@
   // OpenCL v1.2 s6.9.p:
   // Arguments to kernel functions that are declared to be a struct or 
union
   // do not allow OpenCL objects to be passed as elements of the struct or
-  // union.
-  if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||
-  ParamType == InvalidAddrSpacePtrKernelParam) {
+  // union. This restriction was lifted in OpenCL v2.0 with the 
introduction
+  // of SVM.
+  if (ParamType == InvalidAddrSpacePtrKernelParam ||
+  ((ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) &&
+   S.getLangOpts().getOpenCLCompatibleVersion() <= 120)) {
 S.Diag(Param->getLocation(),
diag::err_record_with_pointers_kernel_param)
   << PT->isUnionType()
   << PT;
+  } else if (ParamType == PtrKernelParam || ParamType == 
PtrPtrKernelParam) {
+// and OpenCL version is at-least 2.0, so these types are allowed.
+continue;
   } else {
 S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
   }


Index: clang/test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-parameters.cl
+++ clang/test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,ocl12 %s -triple spir-unknown-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown -cl-std=CL2.0
 
 kernel void half_arg(half x) { } // expected-error{{declaring function parameter of type '__private half' is not allowed; did you forget * ?}}
@@ -87,15 +87,16 @@
 
 
 
-typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' declared here}}
+

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added inline comments.



Comment at: clang/test/SemaOpenCL/invalid-kernel-parameters.cl:90
 
+#if __OPENCL_C_VERSION__ <= CL_VERSION_1_2
 typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' 
declared here}}

yaxunl wrote:
> Ayal wrote:
> > yaxunl wrote:
> > > we should not limit the tests to CL1.2. We should test them with 2.0 to 
> > > make sure there is no diagnostics.
> > > 
> > > To differentiate between 1.2 and 2.0 you can use -verify=ocl12 and 
> > > -verify=ocl20.
> > > 
> > > same as below
> > Using -verify=ocl12 and -verify=ocl20 should probably be done separately as 
> > an NFC patch as it involves many changes IIUC.
> > 
> > The expect's were placed under #ifdef instead (and only them), following 
> > e.g. existing lines 8-12.
> you can use -verify=expected,ocl12 for OCL1.2 and -verify for OCL2.0 then the 
> common diagnostics are kept as 'expected-*' and unchanged. you only need to 
> change the ocl1.2 specific diagnostics to 'ocl12-*'.
> 
> For example, 
> https://github.com/llvm/llvm-project/blob/main/clang/test/SemaOpenCL/atomic-ops.cl
Ah, ok, done, thanks!

(Note that atomic-ops.cl uses only `expected`'s, some of which are placed under 
#if, as done elsewhere in this test file.)


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

https://reviews.llvm.org/D143849

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

[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Ayal Zaks via Phabricator via cfe-commits]

Ayal added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+  // of SVM.
+  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+  (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))

Anastasia wrote:
> Anastasia wrote:
> > Ayal wrote:
> > > Anastasia wrote:
> > > > I think it should be possible to merge this with `if` below to avoid 
> > > > condition duplication.
> > > > 
> > > > 
> > > Sure, but that trades one duplication for another, rather than clearly 
> > > separating the early-continue case early?
> > > 
> > > ```
> > >   if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) {
> > > if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
> > >   continue;
> > > S.Diag(Param->getLocation(),
> > >diag::err_record_with_pointers_kernel_param)
> > >   << PT->isUnionType()
> > >   << PT;
> > >   } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
> > > S.Diag(Param->getLocation(),
> > >diag::err_record_with_pointers_kernel_param)
> > >   << PT->isUnionType()
> > >   << PT;
> > >   } else {
> > > S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << 
> > > PT;
> > > 
> > > ```
> > I am mainly thinking in terms of maintenance if for example someone fixes 
> > one if and forgets another. Or if ifs will get separated by some other code 
> > and then it is not easy to see that the same thing is handled differently 
> > in OpenCL versions. 
> > 
> > Unfortunately we have a lot of those cases, I know this function has early 
> > exists but it is not a common style.
> > 
> > 
> > I was talking about something like:
> > 
> > 
> > ```
> > if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
> > (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
> >   ParamType == InvalidAddrSpacePtrKernelParam)
> > ```
> > 
> > It would also be ok to separate `InvalidAddrSpacePtrKernelParam` since it's 
> > handling different feature.
> Sorry I have forgotten that this part of the code is expected to handle the 
> diagnostics only. The decision that the kernel parameter is wrong is done in 
> `getOpenCLKernelParameterType`. I think you should alter the conditions there 
> to check for OpenCL version and avoid classifying cases you care about as 
> `PtrKernelParam` or `PtrPtrKernelParam`. Then here you wouldn't need this 
> extra if/continue block. 
Hmm, would that be better? This part of the code, namely 
`checkIsValidOpenCLKernelParameter()`, does check the validity of arguments 
classified by `getOpenCLKernelParameterType()` in addition to handling 
diagnostics. E.g., the first case above decides that arguments of 
pointer-to-pointer type are wrong along with proper diagnostics for OpenCL 1.x 
while allowing them for other OpenCL versions.

Struct arguments are simply classified as records by 
getOpenCLKernelParameterType(), whereas this part of the code traverses each 
struct and calls getOpenCLKernelParameterType() on each field - the latter 
seems  unaware if it was invoked on a struct field or not? If it is (made) 
aware, it could indeed return a (new kind of?) invalid type instead of pointer 
type for OpenCL 1.x - how would the right err_record_with_pointers_kernel_param 
diagnostics then be handled? If desired, such refactoring should probably be 
done independent of this fix?


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

https://reviews.llvm.org/D143849

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