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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to