[OpenCL] Refine pipe builtin support

[pxli168]

Summary

Refine the type builtin support as the request with
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160201/148637.html

[yaxunl]

LGTM

lib/Sema/SemaChecking.cpp

339–340

should this follow the convention? e.g.
OpenCL v2.0 s6.13.16.2 -

[pxli168]

lib/Sema/SemaChecking.cpp

339–340

right, this will be more clear.

[yaxunl]

LGTM. Thanks.

[Anastasia]

I think some comments from Richad's feedback are missing, specifically:

• Missing space after comma.
• Why are these conversions performed here rather than in Sema?
• You should attempt to implicitly convert to the desired type here, rather than demanding the right type, to match the normal call semantics. Likewise elsewhere in this patch.

Do you plan to commit them in a separate patch?

Also please add Richard to reviewers list here for the final Ok.

Thanks!

lib/Sema/SemaChecking.cpp

293

Could we use getName() instead?

We could then also move this statement after the switch and just set an error flag here.

[Anastasia]

lib/Sema/SemaChecking.cpp

293

Just to be clear getName() of the attr instead of passing the string (i.e. "read_only").

[pxli168]

Hi Richard/Anastasia,

I replied in the commit email, and here are some explains:

1. Without the space after comma the "//" will be aligned.
2. We want generic prototypes of the builtin functions, and we generate them in CodeGen. So there will be no use to perform conversion in sema for the function prototype.
3. We used generic prototypes and varadic function declration, the conversion in CodeGen use cast and won't change the original type for some arguments that maybe used elsewhere. (Also I do not understand very clear about how to convert in Sema)

Thanks
Xiuli

lib/Sema/SemaChecking.cpp

293

Here we need a different access qualifier then we have. I seems to complicated to generated a useless attribute here just for a name.

[Anastasia]

Related to:

1. I think I would still add space. You can also reformat other lines. There are only 5 lines above.
2. Agree.
3. Feels like may be we should try to see if the passed argument is convertible to the function parameter type.

For example, I see that some builtins in SemaChecking.cpp use DefaultFunctionArrayLvalueConversion to try converting to a pointer type. I am not sure what we could do for other types though especially for the OpenCL types.

@Richard, would you be able to give us more information here:

> +  case 4: {
> +    if (checkOpenCLPipeArg(S, Call))
> +      return true;
> +    // The call with 4 arguments should be
> +    // read/write_pipe(pipe T, reserve_id_t, uint, T*)
> +    // check reserve_id_t
> +    if (!Call->getArg(1)->getType()->isReserveIDT()) {

You should attempt to implicitly convert to the desired type here, rather than demanding the right type, to match the normal call semantics. Likewise elsewhere in this patch.

[Anastasia]

Could you please address number 1 from my previous comment?

Otherwise, I think we should try to proceed quickly here, it will be too hard to merge back in after long delay and also it would be nice to have as many corrections as possible ASAP.

Could we move Richard to subscribers for now, as he can give his feedback later too.

@pekka, do you have any more comments?

[pekka.jaaskelainen]

In D16876#359781, @Anastasia wrote:

@pekka, do you have any more comments?

Nope. Looking forward to finally implementing proper pipe support to pocl.

With the future Clang OpenCL patches I appreciate if you keep adding me to the cc list of the reviews, but it might be pointless to wait for my LGTM as I'm still quite a Clang noob, thus that "LGTM" might not have the "weight" it should have :) In other words, I keep monitoring the patches and will yell if I see something that sticks to my eye, but no point in blocking the progress due to possibly slow acks from my side.

[Anastasia]

In D16876#359786, @pekka.jaaskelainen wrote:

In D16876#359781, @Anastasia wrote:

@pekka, do you have any more comments?

Nope. Looking forward to finally implementing proper pipe support to pocl.

With the future Clang OpenCL patches I appreciate if you keep adding me to the cc list of the reviews, but it might be pointless to wait for my LGTM as I'm still quite a Clang noob, thus that "LGTM" might not have the "weight" it should have :) In other words, I keep monitoring the patches and will yell if I see something that sticks to my eye, but no point in blocking the progress due to possibly slow acks from my side.

Sure, no problem! Thanks!

[pxli168]

1. Make new indent and leave space for the incoming OpenCL C++.
2. Check for the index to see if they are integers.

[yaxunl]

LGTM. Thanks.

[Anastasia]

LGTM!

include/clang/Basic/Builtins.h

 namespace clang {
 class TargetInfo;
 class IdentifierTable;
 class ASTContext;
 class QualType;
 class LangOptions;
 
 enum LanguageID {
-  GNU_LANG = 0x1,  // builtin requires GNU mode.
-  C_LANG = 0x2,    // builtin for c only.
-  CXX_LANG = 0x4,  // builtin for cplusplus only.
-  OBJC_LANG = 0x8, // builtin for objective-c and objective-c++
-  MS_LANG = 0x10,  // builtin requires MS mode.
-  OCLC_LANG = 0x20,// builtin for OpenCL C only.
+  GNU_LANG = 0x1,     // builtin requires GNU mode.
+  C_LANG = 0x2,       // builtin for c only.
+  CXX_LANG = 0x4,     // builtin for cplusplus only.
+  OBJC_LANG = 0x8,    // builtin for objective-c and objective-c++
+  MS_LANG = 0x10,     // builtin requires MS mode.
+  OCLC_LANG = 0x20,   // builtin for OpenCL C only.
   ALL_LANGUAGES = C_LANG | CXX_LANG | OBJC_LANG, // builtin for all languages.
   ALL_GNU_LANGUAGES = ALL_LANGUAGES | GNU_LANG,  // builtin requires GNU mode.
   ALL_MS_LANGUAGES = ALL_LANGUAGES | MS_LANG     // builtin requires MS mode.
 };
 
 namespace Builtin {
 enum ID {
   NotBuiltin  = 0,      // This is not a builtin function.

include/clang/Basic/DiagnosticSemaKinds.td

def err_opencl_pointer_to_type : Error<
   "pointer to type %0 is invalid in OpenCL">;
 def err_opencl_type_can_only_be_used_as_function_parameter : Error <
   "type %0 can only be used as a function parameter in OpenCL">;
 
 // 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)">;
 def err_opencl_builtin_pipe_invalid_access_modifier : Error<
   "invalid pipe access modifier (expecting %0)">;
 
 // OpenCL access qualifier
 def err_opencl_invalid_access_qualifier : Error<
   "access qualifier can only be used for pipe and image type">;

lib/Sema/SemaChecking.cpp

if (!S || !(S->getFlags() & NeededScopeFlags)) {
     SemaRef.Diag(TheCall->getExprLoc(), DiagID)
         << DRE->getDecl()->getIdentifier();
     return true;
   }
 
   return false;
 }
 
-/// Returns readable name for a call.
-static StringRef getFunctionName(CallExpr *Call) {
-  return cast<FunctionDecl>(Call->getCalleeDecl())->getName();
-}
-
 /// Returns OpenCL access qual.
 static OpenCLAccessAttr *getOpenCLArgAccess(const Decl *D) {
-  if (D->hasAttr<OpenCLAccessAttr>())
     return D->getAttr<OpenCLAccessAttr>();
-  return nullptr;
 }
 
 /// Returns true if pipe element type is different from the pointer.
 static bool checkOpenCLPipeArg(Sema &S, CallExpr *Call) {
   const Expr *Arg0 = Call->getArg(0);
   // First argument type should always be pipe.
   if (!Arg0->getType()->isPipeType()) {
     S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_first_arg)
-        << getFunctionName(Call) << Arg0->getSourceRange();
+        << Call->getDirectCallee() << Arg0->getSourceRange();
     return true;
   }
   OpenCLAccessAttr *AccessQual =
       getOpenCLArgAccess(cast<DeclRefExpr>(Arg0)->getDecl());
   // Validates the access qualifier is compatible with the call.
   // OpenCL v2.0 s6.13.16 - The access qualifiers for pipe should only be
   // read_only and write_only, and assumed to be read_only if no qualifier is
   // specified.
-  bool isValid = true;
-  bool ReadOnly = getFunctionName(Call).find("read") != StringRef::npos;
-  if (ReadOnly)
-    isValid = AccessQual == nullptr || AccessQual->isReadOnly();
-  else
-    isValid = AccessQual != nullptr && AccessQual->isWriteOnly();
-  if (!isValid) {
-    const char *AM = ReadOnly ? "read_only" : "write_only";
+  switch (Call->getDirectCallee()->getBuiltinID()) {
+  case Builtin::BIread_pipe:
+  case Builtin::BIreserve_read_pipe:
+  case Builtin::BIcommit_read_pipe:
+  case Builtin::BIwork_group_reserve_read_pipe:
+  case Builtin::BIsub_group_reserve_read_pipe:
+  case Builtin::BIwork_group_commit_read_pipe:
+  case Builtin::BIsub_group_commit_read_pipe:
+    if (!(!AccessQual || AccessQual->isReadOnly())) {
       S.Diag(Arg0->getLocStart(),
              diag::err_opencl_builtin_pipe_invalid_access_modifier)
-        << AM << Arg0->getSourceRange();
+          << "read_only" << Arg0->getSourceRange();
       return true;
     }
-
+    break;
+  case Builtin::BIwrite_pipe:
+  case Builtin::BIreserve_write_pipe:
+  case Builtin::BIcommit_write_pipe:
+  case Builtin::BIwork_group_reserve_write_pipe:
+  case Builtin::BIsub_group_reserve_write_pipe:
+  case Builtin::BIwork_group_commit_write_pipe:
+  case Builtin::BIsub_group_commit_write_pipe:
+    if (!(AccessQual && AccessQual->isWriteOnly())) {
+      S.Diag(Arg0->getLocStart(),
+             diag::err_opencl_builtin_pipe_invalid_access_modifier)
+          << "write_only" << Arg0->getSourceRange();
+      return true;
+    }
+    break;
+  default:
+    break;
+  }
   return false;
 }
 
 /// Returns true if pipe element type is different from the pointer.
 static bool checkOpenCLPipePacketType(Sema &S, CallExpr *Call, unsigned Idx) {
   const Expr *Arg0 = Call->getArg(0);
   const Expr *ArgIdx = Call->getArg(Idx);
   const PipeType *PipeTy = cast<PipeType>(Arg0->getType());
-  const Type *EltTy = PipeTy->getElementType().getTypePtr();
-  const PointerType *ArgTy =
-      dyn_cast<PointerType>(ArgIdx->getType().getTypePtr());
+  const QualType EltTy = PipeTy->getElementType();
+  const PointerType *ArgTy = ArgIdx->getType()->getAs<PointerType>();
   // The Idx argument should be a pointer and the type of the pointer and
   // the type of pipe element should also be the same.
-  if (!ArgTy || EltTy != ArgTy->getPointeeType().getTypePtr()) {
+  if (!ArgTy || S.Context.hasSameType(EltTy, ArgTy->getPointeeType())) {
     S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
-        << getFunctionName(Call)
-        << S.Context.getPointerType(PipeTy->getElementType())
+        << Call->getDirectCallee() << S.Context.getPointerType(EltTy)
         << ArgIdx->getSourceRange();
     return true;
   }
   return false;
 }
 
 // \brief Performs semantic analysis for the read/write_pipe call.
 // \param S Reference to the semantic analyzer.
 // \param Call A pointer to the builtin call.
 // \return True if a semantic error has been found, false otherwise.
 static bool SemaBuiltinRWPipe(Sema &S, CallExpr *Call) {
-  // Two kinds of read/write pipe
-  // From OpenCL C Specification 6.13.16.2 the built-in read/write
-  // functions have following forms.
+  // OpenCL v2.0 s6.13.16.2 - The built-in read/write
+  // functions have two forms.
   switch (Call->getNumArgs()) {
   case 2: {
     if (checkOpenCLPipeArg(S, Call))
       return true;
     // The call with 2 arguments should be
-    // read/write_pipe(pipe T, T*)
-    // check packet type T
+    // read/write_pipe(pipe T, T*).
+    // Check packet type T.
     if (checkOpenCLPipePacketType(S, Call, 1))
       return true;
   } break;
 
   case 4: {
     if (checkOpenCLPipeArg(S, Call))
       return true;
     // The call with 4 arguments should be
-    // read/write_pipe(pipe T, reserve_id_t, uint, T*)
-    // check reserve_id_t
+    // read/write_pipe(pipe T, reserve_id_t, uint, T*).
+    // Check reserve_id_t.
     if (!Call->getArg(1)->getType()->isReserveIDT()) {
       S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
-          << getFunctionName(Call) << S.Context.OCLReserveIDTy
+          << Call->getDirectCallee() << S.Context.OCLReserveIDTy
           << Call->getArg(1)->getSourceRange();
       return true;
     }
 
-    // check the index
+    // Check the index.
     const Expr *Arg2 = Call->getArg(2);
     if (!Arg2->getType()->isIntegerType() &&
         !Arg2->getType()->isUnsignedIntegerType()) {
       S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
-          << getFunctionName(Call) << S.Context.UnsignedIntTy
+          << Call->getDirectCallee() << S.Context.UnsignedIntTy
           << Arg2->getSourceRange();
       return true;
     }
 
-    // check packet type T
+    // Check packet type T.
     if (checkOpenCLPipePacketType(S, Call, 3))
       return true;
   } break;
   default:
     S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_arg_num)
-        << getFunctionName(Call) << Call->getSourceRange();
+        << Call->getDirectCallee() << Call->getSourceRange();
     return true;
   }
 
   return false;
 }
 
 // \brief Performs a semantic analysis on the {work_group_/sub_group_
 //        /_}reserve_{read/write}_pipe
 // \param S Reference to the semantic analyzer.
 // \param Call The call to the builtin function to be analyzed.
 // \return True if a semantic error was found, false otherwise.
 static bool SemaBuiltinReserveRWPipe(Sema &S, CallExpr *Call) {
   if (checkArgCount(S, Call, 2))
     return true;
 
   if (checkOpenCLPipeArg(S, Call))
     return true;
 
-  // check the reserve size
+  // Check the reserve size.
   if (!Call->getArg(1)->getType()->isIntegerType() &&
       !Call->getArg(1)->getType()->isUnsignedIntegerType()) {
     S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
-        << getFunctionName(Call) << S.Context.UnsignedIntTy
+        << Call->getDirectCallee() << S.Context.UnsignedIntTy
         << Call->getArg(1)->getSourceRange();
     return true;
   }
 
   return false;
 }
 
 // \brief Performs a semantic analysis on {work_group_/sub_group_
 //        /_}commit_{read/write}_pipe
 // \param S Reference to the semantic analyzer.
 // \param Call The call to the builtin function to be analyzed.
 // \return True if a semantic error was found, false otherwise.
 static bool SemaBuiltinCommitRWPipe(Sema &S, CallExpr *Call) {
   if (checkArgCount(S, Call, 2))
     return true;
 
   if (checkOpenCLPipeArg(S, Call))
     return true;
 
-  // check reserve_id_t
+  // Check reserve_id_t.
   if (!Call->getArg(1)->getType()->isReserveIDT()) {
     S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
-        << getFunctionName(Call) << S.Context.OCLReserveIDTy
+        << Call->getDirectCallee() << S.Context.OCLReserveIDTy
         << Call->getArg(1)->getSourceRange();
     return true;
   }
 
   return false;
 }
 
 // \brief Performs a semantic analysis on the call to built-in Pipe
 //        Query Functions.
 // \param S Reference to the semantic analyzer.
 // \param Call The call to the builtin function to be analyzed.
 // \return True if a semantic error was found, false otherwise.
 static bool SemaBuiltinPipePackets(Sema &S, CallExpr *Call) {
   if (checkArgCount(S, Call, 1))
     return true;
 
   if (!Call->getArg(0)->getType()->isPipeType()) {
     S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_first_arg)
-        << getFunctionName(Call) << Call->getArg(0)->getSourceRange();
+        << Call->getDirectCallee() << Call->getArg(0)->getSourceRange();
     return true;
   }
 
   return false;
 }
 
 ExprResult
 Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
 
 void test1(read_only pipe int p, global int* ptr){
   int tmp;
   reserve_id_t rid;
 
   // read/write_pipe
-  read_pipe(tmp, p);    // expected-error {{first argument to read_pipe must be a pipe type}}
-  read_pipe(p);   // expected-error {{invalid number of arguments to function: read_pipe}}
-  read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function read_pipe (expecting 'reserve_id_t')}}
-  read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function read_pipe (expecting 'unsigned int')}}
-  read_pipe(p, tmp);   // expected-error {{invalid argument type to function read_pipe (expecting 'int *')}}
+  read_pipe(tmp, p);    // expected-error {{first argument to 'read_pipe' must be a pipe type}}
+  read_pipe(p);   // expected-error {{invalid number of arguments to function: 'read_pipe'}}
+  read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'reserve_id_t')}}
+  read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'unsigned int')}}
+  read_pipe(p, tmp);   // expected-error {{invalid argument type to function 'read_pipe' (expecting 'int *')}}
   write_pipe(p, ptr);    // expected-error {{invalid pipe access modifier (expecting write_only)}}
   write_pipe(p, rid, tmp, ptr);    // expected-error {{invalid pipe access modifier (expecting write_only)}}
 
   // reserve_read/write_pipe
-  reserve_read_pipe(p, ptr);    // expected-error{{invalid argument type to function reserve_read_pipe (expecting 'unsigned int')}}
-  work_group_reserve_read_pipe(tmp, tmp);    // expected-error{{first argument to work_group_reserve_read_pipe must be a pipe type}}
+  reserve_read_pipe(p, ptr);    // expected-error{{invalid argument type to function 'reserve_read_pipe' (expecting 'unsigned int')}}
+  work_group_reserve_read_pipe(tmp, tmp);    // expected-error{{first argument to 'work_group_reserve_read_pipe' must be a pipe type}}
   sub_group_reserve_write_pipe(p, tmp);    // expected-error{{invalid pipe access modifier (expecting write_only)}}
 
   // commit_read/write_pipe
-  commit_read_pipe(tmp, rid);    // expected-error{{first argument to commit_read_pipe must be a pipe type}}
-  work_group_commit_read_pipe(p, tmp);    // expected-error{{invalid argument type to function work_group_commit_read_pipe (expecting 'reserve_id_t')}}
+  commit_read_pipe(tmp, rid);    // expected-error{{first argument to 'commit_read_pipe' must be a pipe type}}
+  work_group_commit_read_pipe(p, tmp);    // expected-error{{invalid argument type to function 'work_group_commit_read_pipe' (expecting 'reserve_id_t')}}
   sub_group_commit_write_pipe(p, tmp);    // expected-error{{invalid pipe access modifier (expecting write_only)}}
 }
 
 void test2(write_only pipe int p, global int* ptr){
   int tmp;
   reserve_id_t rid;
 
   // read/write_pipe
-  write_pipe(tmp, p);    // expected-error {{first argument to write_pipe must be a pipe type}}
-  write_pipe(p);   // expected-error {{invalid number of arguments to function: write_pipe}}
-  write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting 'reserve_id_t')}}
-  write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting 'unsigned int')}}
-  write_pipe(p, tmp);   // expected-error {{invalid argument type to function write_pipe (expecting 'int *')}}
+  write_pipe(tmp, p);    // expected-error {{first argument to 'write_pipe' must be a pipe type}}
+  write_pipe(p);   // expected-error {{invalid number of arguments to function: 'write_pipe'}}
+  write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'reserve_id_t')}}
+  write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'unsigned int')}}
+  write_pipe(p, tmp);   // expected-error {{invalid argument type to function 'write_pipe' (expecting 'int *')}}
   read_pipe(p, ptr);    // expected-error {{invalid pipe access modifier (expecting read_only)}}
   read_pipe(p, rid, tmp, ptr);    // expected-error {{invalid pipe access modifier (expecting read_only)}}
 
   // reserve_read/write_pipe
-  reserve_write_pipe(p, ptr);    // expected-error{{invalid argument type to function reserve_write_pipe (expecting 'unsigned int')}}
-  work_group_reserve_write_pipe(tmp, tmp);    // expected-error{{first argument to work_group_reserve_write_pipe must be a pipe type}}
+  reserve_write_pipe(p, ptr);    // expected-error{{invalid argument type to function 'reserve_write_pipe' (expecting 'unsigned int')}}
+  work_group_reserve_write_pipe(tmp, tmp);    // expected-error{{first argument to 'work_group_reserve_write_pipe' must be a pipe type}}
   sub_group_reserve_read_pipe(p, tmp);    // expected-error{{invalid pipe access modifier (expecting read_only)}}
 
   // commit_read/write_pipe
-  commit_write_pipe(tmp, rid);    // expected-error{{first argument to commit_write_pipe must be a pipe type}}
-  work_group_commit_write_pipe(p, tmp);    // expected-error{{invalid argument type to function work_group_commit_write_pipe (expecting 'reserve_id_t')}}
+  commit_write_pipe(tmp, rid);    // expected-error{{first argument to 'commit_write_pipe' must be a pipe type}}
+  work_group_commit_write_pipe(p, tmp);    // expected-error{{invalid argument type to function 'work_group_commit_write_pipe' (expecting 'reserve_id_t')}}
   sub_group_commit_read_pipe(p, tmp);    // expected-error{{invalid pipe access modifier (expecting read_only)}}
 }
 
 void test3(){
   int tmp;
-  get_pipe_num_packets(tmp);    // expected-error {{first argument to get_pipe_num_packets must be a pipe type}}
-  get_pipe_max_packets(tmp);    // expected-error {{first argument to get_pipe_max_packets must be a pipe type}}
+  get_pipe_num_packets(tmp);    // expected-error {{first argument to 'get_pipe_num_packets' must be a pipe type}}
+  get_pipe_max_packets(tmp);    // expected-error {{first argument to 'get_pipe_max_packets' must be a pipe type}}
 }