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

[Yaxun Liu via cfe-commits]

yaxunl marked 5 inline comments as done.
yaxunl added a comment.

> 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.


These builtin functions can be lowered by a module pass which tracing the argument to determine the real addr space. In that case it does not matter that it can take infinite number of names.

However, there may be platforms on which these builtin functions are implemented in runtime library. I agree code generated as such will cause difficulty for them. Therefore I will emit call to i8 addrspace(1)* to_global (i8* addrspace(4)) and insert bitcasts. Also the name will not be mangled.


================
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();
----------------
Anastasia wrote:
> 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. 
The return type is generated based on the function and argument type. It always matches the argument type and has right AS. If the context of this call expr expects a different type, diagnostics will be emitted by the enclosing context.

================
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(
----------------
Anastasia wrote:
> 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.
In that case the type of the call expr does not need to be dynamically generated.

In this case, the type of the call expr depends on the argument type. If we don't dynamically generate the prototype of the builtin function, we have to insert bitcast, which results in an AST which is no longer a call expr and the diagnostics will become ugly.


http://reviews.llvm.org/D19932