[PATCH] D19932: [OpenCL] Add to_{global|local|private} builtin functions.

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

In http://reviews.llvm.org/D19932#422374, @yaxunl wrote:

> In http://reviews.llvm.org/D19932#421961, @pxli168 wrote:
>
> > Could we output a generic function in CodeGen?
> >  This seems to have no big difference to have a lot of declaration in an opencl c header file.
>
>
> What return type and argument type to use would you suggest for this generic function? If it is something like
>
>   i8 addrspace(1)* to_global(i8 addrspace(4)*)
>   
>
> then bitcasts will be needed, which is what we want to avoid.
>
> These functions accept pointers to arbitrary user types, so they cannot be declared in a header file.


I agree with Xiuli, if we don't have generic implementation with i8*,  the libraries would have to contain every possible implementation of each AS conversion builtin including user defined types (which we don't even know beforehand).

I think that bitcast is not an issue in general as soon as passed type is casted from and back to the original type. This way we will disallow loosing the type information if let's say we compile:

  int *ptr = ...
  to_global(ptr) -> the return type is global int* (and not global void*)
   

Because we can create a bitcast to the original type.


================
Comment at: include/clang/Basic/Builtins.def:1292
@@ +1291,3 @@
+// OpenCL v2.0 s6.13.9 - Address space qualifier functions.
+LANGBUILTIN(to_global, "v*v*", "tn", OCLC_LANG)
+LANGBUILTIN(to_local, "v*v*", "tn", OCLC_LANG)
----------------
I think we should not enable those builtins for all OpenCL versions as it prevents using those identifiers. I don't think they are reserved in the earlier OpenCL versions?

But I have a patch fixing it for all CL2.0 builtins. I can upload later. No need to fix now!

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2127
@@ -2126,1 +2126,3 @@
 
+  // OpenCL v2.0 s6.13.9 Address space qualifier functions.
+  case Builtin::BIto_global:
----------------
Could you add "-" before "Address ..." to match general style.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2138
@@ +2137,3 @@
+        Builder.CreateCall(CGM.CreateRuntimeFunction(FTy,
+          CGM.getMangledName(E->getDirectCallee())), {Arg0}));
+  }
----------------
I don't think mangling would work here at all. See my general comment to the generated prototype discussion.

================
Comment at: lib/Sema/SemaChecking.cpp:480
@@ +479,3 @@
+      .getQualifiers().getAddressSpace() == LangAS::opencl_constant) {
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_to_addr_invalid_arg)
+        << Call->getArg(0) << Call->getDirectCallee() << Call->getSourceRange();
----------------
Should we also check that argument and return pointer type match?

Also we should probably check that the return type is in the right AS. 

================
Comment at: lib/Sema/SemaExpr.cpp:5222
@@ -5166,5 +5221,3 @@
     if (FDecl && FDecl->getBuiltinID()) {
-      // Rewrite the function decl for this builtin by replacing parameters
-      // with no explicit address space with the address space of the arguments
-      // in ArgExprs.
-      if ((FDecl = rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs))) {
+      // Rewrite the function decl for OpenCL to_addr builtin.
+      if (FunctionDecl *NFDecl = rewriteBuiltinFunctionDeclForOpenCLToAddr(
----------------
Is there any reason not to do normal IR CodeGen in CGBuiltin.cpp like we did for pipes:
http://reviews.llvm.org/D15914

Otherwise, it seems like we add an extra overhead for all the builtins and it doesn't seem any simpler code either.


http://reviews.llvm.org/D19932