[PATCH] D15914: [OpenCL] Pipe builtin functions

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

Do you have any tests? We won't be able to commit without. Also having them would help understanding what this modification does.


================
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)
----------------
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"

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

================
Comment at: lib/Sema/SemaChecking.cpp:268
@@ +267,3 @@
+static OpenCLImageAccessAttr *getOpenCLImageAcces(const Decl *D) {
+  if (D->hasAttr<OpenCLImageAccessAttr>())
+    return D->getAttr<OpenCLImageAccessAttr>();
----------------
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.

================
Comment at: lib/Sema/SemaChecking.cpp:282
@@ +281,3 @@
+  }
+  OpenCLImageAccessAttr *AccessQual =
+      getOpenCLImageAcces(cast<DeclRefExpr>(Arg0)->getDecl());
----------------
Ah yes, I see. It should be fine!


http://reviews.llvm.org/D15914