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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
