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

[Yaxun Liu via cfe-commits]

yaxunl marked 8 inline comments as done.

================
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:
> 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 *'}}
I added the above test. The current implementation is able to diagnose it.

Your suggested approach also works, so I will remove dynamic generation of to_addr declarations since it seems to be an overkill.

================
Comment at: test/SemaOpenCL/to_addr_builtin.cl:25
@@ +24,3 @@
+  glob = to_global(con);
+#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
+  // expected-error at -2{{'to_global' needs OpenCL version 2.0 or above}}
----------------
pxli168 wrote:
> yaxunl wrote:
> > pxli168 wrote:
> > > But in Spec OpenCL V2.0 s6.13.9 the description for to_global is:
> > > 
> > > > Returns a pointer that points to a region in the global address space if to_global can cast ptr to the global address space. Otherwise it returns NULL.
> > > 
> > I think this is only for valid call expression. For constant pointer, it is an invalid call expression due to s6.5.5, so an error of invalid argument should be emitted.
> > 
> > If we allow passing constant pointer to this function, we violates s6.5.5.
> >A pointer that points to the constant address space cannot be cast or implicitly converted to the generic address space.
>  
> So if we can not cast, we maybe should return NULL?
> @Anastasia What do you think about this?
> 
> 
What the function returns is implemented by builtin library and Clang does not care about that. Clang only does syntax checking, type checking, etc. Since this is still a function, it is subject to all these rules. Unless we want to exempt this function from the type-checking rules. However I don't think the spec implies that.


http://reviews.llvm.org/D19932