[PATCH] D15914: [OpenCL] Pipe builtin functions

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: include/clang/Basic/Builtins.def:1255
@@ -1254,1 +1254,3 @@
 
+// OpenCL v2.0 s6.13.16, s9.17.3.5 -- Pipe functions.
+// We need the generic prototype, since the packet type could be anything.
----------------
Could you remove one -?

================
Comment at: include/clang/Basic/Builtins.h:39
@@ -38,2 +38,3 @@
   MS_LANG = 0x10,  // builtin requires MS mode.
+  OCLC_LANG = 0x20,// builtin for OpenCL C only.
   ALL_LANGUAGES = C_LANG | CXX_LANG | OBJC_LANG, // builtin for all languages.
----------------
May be: OCLC -> OCL

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2002
@@ +2001,3 @@
+      Value *BCast = Builder.CreatePointerCast(Arg3, I8PTy);
+      // We know the third argument is an integer type (Verified by Sema), but
+      // we may need to cast it to i32.
----------------
remove "(Verified by Sema)"

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2033
@@ +2032,3 @@
+          *Arg1 = EmitScalarExpr(E->getArg(1));
+    llvm::Type *ReservedIDTy = ConvertType(getContext().OCLReserveIDTy);
+
----------------
Yes. I do believe that a pass would recover this, but on the other hand it is an extra step. Every such step can potentially add to compilation time which can easily be avoided by already generating code in the format that directly sets the right value.

> "Yours optimizes automatically but requires special casing (when calling a builtin, not user function, add this magic parameter)"
I am not sure what is special casing. All pipe functions are Clang builtins now and will be generated to calls to internal functions (for those we can define the format the way we need it to be). 

Anyways, I see what you mean this is an optimization and not needed for supporting correct functionality. I still think it's nicer not to force compiler developer to create extra passes for something we can provide in a suitable format from the beginning.

I don't want to block this change for that reason though. This is a small modification and so can be done later as well.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2039
@@ +2038,3 @@
+        ReservedIDTy, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
+    // We know the second argument is an integer type (Verified by Sema), but
+    // we may need to cast it to i32.
----------------
remove "(Verified by Sema)"

================
Comment at: lib/Sema/SemaChecking.cpp:267
@@ +266,3 @@
+/// Returns OpenCL access qual.
+// TODO:Refine OpenCLImageAccessAttr to OpenCLArgAccessAttr since pipe can use
+// it too
----------------
Add space after TODO:

================
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;
----------------
Why the TODO here?

================
Comment at: lib/Sema/SemaChecking.cpp:769
@@ +768,3 @@
+    // We need to override the return type of the reserve pipe functions.
+    TheCall->setType(Context.OCLReserveIDTy);
+    break;
----------------
Could you improve the comment? Not clear why do we need to override the type...

================
Comment at: test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl:5
@@ +4,3 @@
+  int tmp;
+  reserve_id_t rid;
+
----------------
We should tests all code added to the compiler.

================
Comment at: test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl:33
@@ +32,3 @@
+  write_pipe(tmp, p);    // expected-error {{first argument to write_pipe must be a pipe type}}
+  write_pipe(p);   // expected-error {{invalid number of arguments to function: write_pipe}}
+  write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'reserve_id_t')}}
----------------
remove : from error string

================
Comment at: test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl:34
@@ +33,3 @@
+  write_pipe(p);   // expected-error {{invalid number of arguments to function: write_pipe}}
+  write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'reserve_id_t')}}
+  write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'unsigned int')}}
----------------
remove : from error string

================
Comment at: test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl:35
@@ +34,3 @@
+  write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'reserve_id_t')}}
+  write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'unsigned int')}}
+  write_pipe(p, tmp);   // expected-error {{invalid argument type to function write_pipe (expecting: 'int *')}}
----------------
remove : from error string

================
Comment at: test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl:36
@@ +35,3 @@
+  write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'unsigned int')}}
+  write_pipe(p, tmp);   // expected-error {{invalid argument type to function write_pipe (expecting: 'int *')}}
+  read_pipe(p, ptr);    // expected-error {{invalid pipe access modifier (expecting read_only)}}
----------------
remove : from error string


http://reviews.llvm.org/D15914