[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