[OpenCL] Improve diagnostic of argument in address space conversion builtins

[AlistairD]

Summary

This change adds a warning if a parameter is passed with named address space to parameter that is in generic address space.

For example:

int i;
to_private(&i); //generate warning as conversion from private to private is redundant.

[yaxunl]

Is this a feature requested by users?

I can understand that this may be useful to pinpoint some conversions which are potentially harmful to performance. However such conversions can often be eliminated by optimization, which makes this warning less useful.

On the other hand, too many warnings is a nuance to users, therefore I am wondering whether this warning should be off by default.

Do you have any chance to have any feedback from users about how useful or intrusive this warning is.

[yaxunl]

include/clang/Basic/DiagnosticSemaKinds.td

8595

How about change this to:

may cause dynamic conversion affecting performance

[Anastasia]

In D51411#1217477, @yaxunl wrote:

Is this a feature requested by users?

I can understand that this may be useful to pinpoint some conversions which are potentially harmful to performance. However such conversions can often be eliminated by optimization, which makes this warning less useful.

On the other hand, too many warnings is a nuance to users, therefore I am wondering whether this warning should be off by default.

Do you have any chance to have any feedback from users about how useful or intrusive this warning is.

Hi Sam, no feedback from the user. If you think this is not that useful we won't commit or make is off by default as you suggested. Let us know what you think makes more sense.

[yaxunl]

In D51411#1224615, @Anastasia wrote:

In D51411#1217477, @yaxunl wrote:

Is this a feature requested by users?

I can understand that this may be useful to pinpoint some conversions which are potentially harmful to performance. However such conversions can often be eliminated by optimization, which makes this warning less useful.

On the other hand, too many warnings is a nuance to users, therefore I am wondering whether this warning should be off by default.

Do you have any chance to have any feedback from users about how useful or intrusive this warning is.

Hi Sam, no feedback from the user. If you think this is not that useful we won't commit or make is off by default as you suggested. Let us know what you think makes more sense.

I suggest to make it off by default. Since implicit casting to generic pointer is allowed by spec, we only want to see this warning when we want to debug performance issues.

[AlistairD]

This comment was removed by AlistairD.

[AlistairD]

Reworded warning message, switched warning off by default, and added it to -Wconversion group

[Anastasia]

LGTM!

include/clang/Basic/DiagnosticSemaKinds.td

def err_opencl_type_can_only_be_used_as_function_parameter : Error <
   "type %0 can only be used as a function parameter in OpenCL">;
 def warn_opencl_attr_deprecated_ignored : Warning <
   "%0 attribute is deprecated and ignored in OpenCL version %1">,
   InGroup<IgnoredAttributes>;
 def err_opencl_variadic_function : Error<
   "invalid prototype, variadic arguments are not allowed in OpenCL">;
 def err_opencl_requires_extension : Error<
   "use of %select{type|declaration}0 %1 requires %2 extension to be enabled">;
+def warn_opencl_generic_address_space_arg : Warning<
+  "passing non-generic address space pointer to %0"
+  " may cause dynamic conversion affecting performance">,
+  InGroup<Diag<"generic-address-conversion">>, DefaultIgnore;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
 def err_opencl_builtin_pipe_first_arg : Error<
   "first argument to %0 must be a pipe type">;
 def err_opencl_builtin_pipe_arg_num : Error<
   "invalid number of arguments to function: %0">;
 def err_opencl_builtin_pipe_invalid_arg : Error<
   "invalid argument type to function %0 (expecting %1 having %2)">;

lib/Sema/SemaChecking.cpp

static bool SemaOpenCLBuiltinToAddr(Sema &S, unsigned BuiltinID,
   auto RT = Call->getArg(0)->getType();
   if (!RT->isPointerType() || RT->getPointeeType()
       .getAddressSpace() == LangAS::opencl_constant) {
     S.Diag(Call->getBeginLoc(), diag::err_opencl_builtin_to_addr_invalid_arg)
         << Call->getArg(0) << Call->getDirectCallee() << Call->getSourceRange();
     return true;
   }
 
+  if (RT->getPointeeType().getAddressSpace() != LangAS::opencl_generic) {
+    S.Diag(Call->getArg(0)->getLocStart(),
+           diag::warn_opencl_generic_address_space_arg)
+        << Call->getDirectCallee()->getNameInfo().getAsString()
+        << Call->getArg(0)->getSourceRange();
+  }
+
   RT = RT->getPointeeType();
   auto Qual = RT.getQualifiers();
   switch (BuiltinID) {
   case Builtin::BIto_global:
     Qual.setAddressSpace(LangAS::opencl_global);
     break;
   case Builtin::BIto_local:
     Qual.setAddressSpace(LangAS::opencl_local);

test/SemaOpenCL/to_addr_builtin.cl

 // RUN: %clang_cc1 -verify -fsyntax-only %s
-// RUN: %clang_cc1 -verify -fsyntax-only -cl-std=CL2.0 %s
+// RUN: %clang_cc1 -Wgeneric-address-conversion -verify -fsyntax-only -cl-std=CL2.0 %s
 
 void test(void) {
   global int *glob;
   local int *loc;
   constant int *con;
   typedef constant int const_int_ty;
   const_int_ty *con_typedef;
 
#else
   // expected-error@-4{{invalid argument con_typedef to function: 'to_global', expecting a generic pointer argument}}
 #endif
 
   loc = to_global(glob);
 #if __OPENCL_C_VERSION__ < CL_VERSION_2_0
   // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__local int *' from 'int'}}
 #else
   // expected-error@-4{{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  // expected-warning@-5{{passing non-generic address space pointer to to_global may cause dynamic conversion affecting performance}}
 #endif
 
   global char *glob_c = to_global(loc);
 #if __OPENCL_C_VERSION__ < CL_VERSION_2_0
   // expected-warning@-2{{incompatible integer to pointer conversion initializing '__global char *' with an expression of type 'int'}}
 #else
   // expected-warning@-4{{incompatible pointer types initializing '__global char *' with an expression of type '__global int *'}}
+  // expected-warning@-5{{passing non-generic address space pointer to to_global may cause dynamic conversion affecting performance}}
 #endif
 
 }