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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
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();
----------------
I just think you should simply set the function return type in line 487 based on the passed argument type (RT variable declared in line 477).

The situation I would like us to avoid here is:

  int *gptr = ..;
  local char *lptr = to_local(gptr);

So basically by calling to_local here, we allow conversion of not only an address space but also an underlaying type silently. 

Could we add a test to make sure it's not happening.

Similarly, in C such situations trigger a warning:
  int *ptr1 = ...;
  char *ptr2 = ptr1; //expected-warning{{incompatible pointer types initializing 'char *' with an expression of type 'int *'}}

================
Comment at: lib/Sema/SemaExpr.cpp:5222-5232
@@ -5166,6 +5221,13 @@
     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(
+        this, Context, FDecl, ArgExprs))
+        FDecl = NFDecl;
+      else {
+        // Rewrite the function decl for this builtin by replacing parameters
+        // with no explicit address space with the address space of the arguments
+        // in ArgExprs.
+        FDecl = rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs);
+      }
+      if (FDecl) {
         NDecl = FDecl;
----------------
I still don't understand the issue here. 

In my understanding what we need is:
(1) Check the passed argument is a pointer and set return type to match the pointer type exactly in SemaChecking.
(2) Generate to_addr with i8* in CGBuiltin casting to and back the original type used in the call.

See my comment to lib/Sema/SemaChecking.cpp. I think if you set the return type there correctly we won't need this change here.

================
Comment at: test/SemaOpenCL/to_addr_builtin.cl:21
@@ +20,3 @@
+#else
+  // expected-error at -4{{invalid argument x to function: 'to_global'}}
+#endif
----------------
May be we could add here something like: expecting a generic pointer type...


http://reviews.llvm.org/D19932