[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 <https://reviews.llvm.org/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