[PATCH] D15914: [OpenCL] Pipe builtin functions

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

A few nitpicks here, otherwise it seems we are in a good shape. I hope this will hit trunk soon. :)


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1966
@@ -1965,1 +1965,3 @@
   }
+
+  case Builtin::BIread_pipe:
----------------
Can you put a comment with the spec reference here.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1974
@@ +1973,3 @@
+    // Type of the generic packet parameter.
+    llvm::Type *I8PTy = llvm::PointerType::get(llvm::Type::getInt8Ty(Ctxt), 4U);
+
----------------
Please, avoid hard coded constants.

You can use getTargetAddressSpace() method of ASTContext to obtain a generic AS number, something like:
    unsigned GenericAS = getContext().getTargetAddressSpace(LangAS::opencl_generic);
    llvm::Type Int8Ty = ...(..., GenericAS);

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1978
@@ +1977,3 @@
+    if (2U == E->getNumArgs()) {
+      // The name is hard-coded for now, since we would like to avoid the
+      // AST rewrite for the infinite user define types.
----------------
I don't think this comment is any relevant and can be removed!

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1983
@@ +1982,3 @@
+      // Re-Creating the function type for this call, since the original type
+      // is variadic, we make them to be generic type.
+      llvm::Type *ArgTys[] = {Arg0->getType(), I8PTy};
----------------
the type is not really variadic! May be better to say:

"Creating a generic function type to be able to call with any builtin or user defined type."

================
Comment at: lib/Sema/SemaChecking.cpp:286
@@ +285,3 @@
+      getOpenCLArgAccess(cast<DeclRefExpr>(Arg0)->getDecl());
+  // Validates the access modifier is compatible with the call.
+  // From OpenCL C Specification 6.13.16 the access qualifiers for
----------------
modifier -> qualifier

================
Comment at: lib/Sema/SemaChecking.cpp:394
@@ +393,3 @@
+
+  // check the reverse size
+  if (!Call->getArg(1)->getType()->isIntegerType() &&
----------------
reverse -> reserve

================
Comment at: lib/Sema/SemaChecking.cpp:434
@@ +433,3 @@
+// \return True if a semantic error was found, false otherwise.
+static bool SemaBuiltinPipePacketes(Sema &S, CallExpr *Call) {
+  if (checkArgCount(S, Call, 1))
----------------
Packetes -> Packets

================
Comment at: lib/Sema/SemaChecking.cpp:754
@@ +753,3 @@
+  case Builtin::BIwrite_pipe:
+    // Since those two functions are declared with var args, therefore we need
+    // a semantic check for the argument.
----------------
remove "therefore"

================
Comment at: lib/Sema/SemaChecking.cpp:768
@@ +767,3 @@
+    // Since return type of reserve_read/write_pipe built-in function is
+    // reserve_id_t, which is not defined in the builtin def, we used int as
+    // return type and need to override the return type of these functions
----------------
builtin def -> builtin def file


http://reviews.llvm.org/D15914