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

[Ayal]

Summary

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

[Anastasia]

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.

clang/lib/Sema/SemaDecl.cpp

9493

To be honest I feel like it was a bug fix? Do you happen to have any record of the fix?

9494

I think it should be possible to merge this with if below to avoid condition duplication.

[Ayal]

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.

clang/lib/Sema/SemaDecl.cpp

9493

Not sure what was the alleged bug nor its fix, but in any case I have no record thereof..

9494

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;

[Anastasia]

clang/lib/Sema/SemaDecl.cpp

9494

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.

[yaxunl]

clang/test/SemaOpenCL/invalid-kernel-parameters.cl

90

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

[Ayal]

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

[Ayal]

clang/test/SemaOpenCL/invalid-kernel-parameters.cl

90

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.

[yaxunl]

clang/test/SemaOpenCL/invalid-kernel-parameters.cl

90

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

[Ayal]

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

[Ayal]

clang/test/SemaOpenCL/invalid-kernel-parameters.cl

90

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.)

[Anastasia]

clang/lib/Sema/SemaDecl.cpp

9494

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.

[Ayal]

clang/lib/Sema/SemaDecl.cpp

9494

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?

[Anastasia]

clang/lib/Sema/SemaDecl.cpp

9494

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.

[Ayal]

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.

[Ayal]

clang/lib/Sema/SemaDecl.cpp

9494

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?

[Anastasia]

LGTM! Thanks

clang/lib/Sema/SemaDecl.cpp

for (const auto *FD : RD->fields()) {
       if (ParamType == RecordKernelParam) {
         VisitStack.push_back(FD);
         continue;
       }
 
       // 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(),
                diag::err_record_with_pointers_kernel_param)
           << PT->isUnionType()
           << PT;
       } else {
         S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;

clang/test/SemaOpenCL/invalid-kernel-parameters.cl

 {
   bool x; // expected-note{{field of illegal type 'bool' declared here}}
 } ContainsBool;
 
 kernel void bool_in_struct_arg(ContainsBool x) { } // expected-error{{'__private ContainsBool' (aka '__private struct ContainsBool') cannot be used as the type of a kernel parameter}}
 
 
 
+#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
   // 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}}
 } 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}}
 {
   int* ptrField; // expected-note-re{{field of illegal pointer type '__{{private|generic}} int *' declared here}}
 } Foo;
 
 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}}
 } FooUnion;
 
 kernel void pointer_in_union_arg(FooUnion arg) { }// expected-error{{union kernel parameters may not contain pointers}}
 
 typedef struct NestedPointer // expected-note 2 {{within field of type 'NestedPointer' declared here}}