[PATCH] D15914: [OpenCL] Pipe builtin functions

[Anastasia Stulova via cfe-commits]

Anastasia 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)
----------------
Ok, this way it means variable number of arg which isn't right either. But I see some other builtins do the same.

I think a better approach would be to add pipe as a modifier and a way to represent any gentype in builtins declarations here to be able to specify a signature properly. Although it seems it won't be used for anything but documentation purposes.

Also adding the overloaded variant for read/write_pipes explicitly might be a good idea (similar to sync_fetch_and_add).

But I see that not all builtins follow this approach, so it's up to you.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2033
@@ +2032,3 @@
+          *Arg1 = EmitScalarExpr(E->getArg(1));
+    llvm::Type *ReservedIDTy = ConvertType(getContext().OCLReserveIDTy);
+
----------------
Why do we need to modify user defined functions? Only builtins will have this extra parameter.

I think packet size would be useful for builtin implementation to know the number of bytes to be read/written. I don't see how to implement it correctly otherwise. As mentioned earlier relying on the metadata is not a conventional compilation approach and should be avoided if possible.

================
Comment at: lib/Sema/SemaChecking.cpp:268
@@ +267,3 @@
+// TODO:Refine OpenCLImageAccessAttr to OpenCLArgAccessAttr since pipe can use
+// it too
+static OpenCLImageAccessAttr *getOpenCLArgAccess(const Decl *D) {
----------------
Yes, they should. It seems however it has already been checked here?

================
Comment at: lib/Sema/SemaChecking.cpp:274
@@ +273,3 @@
+}
+
+/// Returns true if pipe element type is different from the pointer.
----------------
Makes sense!

================
Comment at: lib/Sema/SemaChecking.cpp:291
@@ +290,3 @@
+  bool isValid = true;
+  // TODO: For all pipe built-in read is for read_only?
+  bool ReadOnly = getFunctionName(Call).find("read") != StringRef::npos;
----------------
I agree the spec doesn't require that checking but I think it's just being imprecise in the description. If you are in doubt you can raise a bug with Khronos to clarify. This might result in additional latency.

I think it makes sense though to check all of them.

================
Comment at: test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl:4
@@ +3,3 @@
+void test(read_only pipe int p, global int* ptr){
+  int tmp;
+  read_pipe(tmp, p);    // expected-error {{first argument to read_pipe must be a pipe type}}
----------------
I think we need to test all the builtins!


http://reviews.llvm.org/D15914