[PATCH] D15914: [OpenCL] Pipe builtin functions

[Xiuli PAN via cfe-commits]

pxli168 added inline comments.

================
Comment at: include/clang/Basic/Builtins.def:1260
@@ +1259,3 @@
+
+LANGBUILTIN(reserve_read_pipe, "i.", "tn", OCLC_LANG)
+LANGBUILTIN(reserve_write_pipe, "i.", "tn", OCLC_LANG)
----------------
Anastasia wrote:
> That's right! I am just saying that we don't have to make it variadic, so you can change "v." -> "v" and "i." -> "i"
If that means there is no arguments, and may be misleading.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2033
@@ +2032,3 @@
+    else if (BuiltinID == Builtin::BIwork_group_reserve_write_pipe)
+      Name = "_Z29work_group_reserve_write_pipePU3AS110ocl_pipe_tji";
+    else if (BuiltinID == Builtin::BIsub_group_reserve_read_pipe)
----------------
pekka.jaaskelainen wrote:
> Anastasia wrote:
> > I am still not convinced we need to mangle here at all. It seems we mainly have only one prototype for those functions apart from read/write_pipe in which case we can generate two versions of that function:
> > 
> > read_pipe_2(...) - for two parameters case
> > read_pipe_4(...) - for four parameters case
> > 
> > Having a mangler would complicate the implementation. And if used it would be better to adhere to conventional scheme  (i.e. calling getMangledName()). But that won't work here as we are generating parameters different from originally specified. I don't see any need or benefit in mangling at the moment since the function signatures are all known here.
> > 
> > Regarding the packet size, I prefer to pass it in. This way we can allow simple BIF implementation without using any metadata magic. Usage of metadata often implies adding non-conventional compilation mechanisms somewhere that are best to avoid.
> We just need to remember that passing the packet size as an argument means touching all the user functions having pipes as arguments too.  
> 
> I'm still in the understanding that the compile time packet size info is needed only for optimization purposes, and thus could be a metadata which your implementation can just ignore for starters, or convert to a parameter before code emission? Touching the initial function finger prints for optimization reasons sounds a bit too intrusive. I'd rather leave the decision what to do with it to consumer of the Clang's output.
That could be fine I think, the function name do not need mangle if we use generic type.

================
Comment at: lib/Sema/SemaChecking.cpp:268
@@ +267,3 @@
+static OpenCLImageAccessAttr *getOpenCLImageAcces(const Decl *D) {
+  if (D->hasAttr<OpenCLImageAccessAttr>())
+    return D->getAttr<OpenCLImageAccessAttr>();
----------------
Anastasia wrote:
> Let's rename this new function in this commit and I agree with renaming an attribute later on. May be leaving TODO explaining that would be good.
OK, it will make the function more easy to understand. And I will add a FIXME or TODO here to explain the misleading imageaccess.

And how do you think about the access check for the work_group functions?
The specification does not say clearly, but it seems they should also obey the same rule with read/write_pipe.


http://reviews.llvm.org/D15914