[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 2.0 Pipe functions.
+// We need the variadic prototype, since the packet type could be anything.
----------------
Could we put reference to spec section?

================
Comment at: include/clang/Basic/Builtins.def:1256
@@ +1255,3 @@
+// OpenCL 2.0 Pipe functions.
+// We need the variadic prototype, since the packet type could be anything.
+BUILTIN(read_pipe, "i.", "tn")
----------------
Is variadic right here? Should be generic perhaps? 

================
Comment at: include/clang/Basic/Builtins.def:1257
@@ +1256,3 @@
+// We need the variadic prototype, since the packet type could be anything.
+BUILTIN(read_pipe, "i.", "tn")
+BUILTIN(write_pipe, "i.", "tn")
----------------
I think it would make sense to have this as LANGBUILTIN as it's only available in OpenCL.

================
Comment at: include/clang/Basic/Builtins.def:1260
@@ +1259,3 @@
+
+BUILTIN(reserve_read_pipe, "i.", "tn")
+BUILTIN(reserve_write_pipe, "i.", "tn")
----------------
>From reserve_read_pipe onwards the builtins have a fixed number of arguments, and don't need to be variadic.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7643
@@ -7642,1 +7642,3 @@
   "pipes packet types cannot be of reference type">;
+// Builtin pipe
+def err_builtin_pipe_first_arg : Error<
----------------
add OpenCL v2.0

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2028
@@ +2027,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.
----------------
Closing ) is missing.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2033
@@ +2032,3 @@
+      return RValue::get(
+          Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name),
+                             {Arg0, Arg1, Arg2, BCast, PacketSize}));
----------------
So why do we need to mangle at all since the generated function signature will always have the same parameter types?

================
Comment at: lib/CodeGen/CGBuiltin.cpp:2081
@@ +2080,3 @@
+      Arg1 = Builder.CreateZExtOrTrunc(Arg1, Int32Ty);
+    return RValue::get(Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name),
+                                          {Arg0, Arg1, PacketSize}));
----------------
So why do we need to mangle at all since the generated function signatures will always have the same parameter types?

================
Comment at: lib/CodeGen/CGOpenCLRuntime.h:55
@@ -50,1 +54,3 @@
 };
+class Ocl20Mangler {
+public:
----------------
I am not very convinced on this solution of implementing a separate mangler. Also how would it play together with existing mangling schemes?

Additionally how would definitions of builtin with user defined types appear in the BIF libraries? I am not clear at the moment.

One approach would be to just generate calls that would always use generic types:  opaque type for pipe and void* for a packet and avoid the mangling completely. That's what I think is done already (see previous comment), just not clear why mangling is added too.

================
Comment at: lib/Sema/SemaChecking.cpp:267
@@ +266,3 @@
+/// Returns OpenCL access qual.
+static OpenCLImageAccessAttr *getOpenCLImageAcces(const Decl *D) {
+  if (D->hasAttr<OpenCLImageAccessAttr>())
----------------
Not sure why we have an image access here?

================
Comment at: lib/Sema/SemaChecking.cpp:268
@@ +267,3 @@
+static OpenCLImageAccessAttr *getOpenCLImageAcces(const Decl *D) {
+  if (D->hasAttr<OpenCLImageAccessAttr>())
+    return D->getAttr<OpenCLImageAccessAttr>();
----------------
What happens if access qualifier is not provided?

According to spec it should default to read_only!

================
Comment at: lib/Sema/SemaChecking.cpp:274
@@ +273,3 @@
+/// Returns true if pipe element type is different from the pointer.
+static bool checkOpenCLPipeArg(Sema &S, CallExpr *call) {
+  const Expr *Arg0 = call->getArg(0);
----------------
Should we also be checking that read_write is not used with pipe?

================
Comment at: lib/Sema/SemaChecking.cpp:282
@@ +281,3 @@
+  }
+  OpenCLImageAccessAttr *AccessQual =
+      getOpenCLImageAcces(cast<DeclRefExpr>(Arg0)->getDecl());
----------------
Should be a generic name rather than image!

================
Comment at: lib/Sema/SemaChecking.cpp:291
@@ +290,3 @@
+  if (getFunctionName(call).find("read") != StringRef::npos)
+    // if (getFunctionName(call).startswith("read"))
+    isValid = AccessQual == nullptr || AccessQual->isReadOnly();
----------------
Remove this commented code

================
Comment at: lib/Sema/SemaChecking.cpp:296
@@ +295,3 @@
+  if (!isValid) {
+    bool ReadOnly = getFunctionName(call).startswith("read");
+    const char *AM = ReadOnly ? "read_only" : "write_only";
----------------
Should you be checking find() instead of startwith()? 

================
Comment at: lib/Sema/SemaChecking.cpp:297
@@ +296,3 @@
+    bool ReadOnly = getFunctionName(call).startswith("read");
+    const char *AM = ReadOnly ? "read_only" : "write_only";
+    S.Diag(Arg0->getLocStart(), diag::err_builtin_pipe_invalid_access_modifier)
----------------
Could move this in the above if statement to avoid rechecking.

================
Comment at: lib/Sema/SemaChecking.cpp:307
@@ +306,3 @@
+/// Returns true if pipe element type is different from the pointer.
+static bool checkOpenCLPipePacketType(Sema &S, CallExpr *call, unsigned idx) {
+  const Expr *Arg0 = call->getArg(0);
----------------
To adhere the coding standard:
call -> Call, idx -> Idx

Please, check other places too.

================
Comment at: lib/Sema/SemaChecking.cpp:309
@@ +308,3 @@
+  const Expr *Arg0 = call->getArg(0);
+  const Expr *Argidx = call->getArg(idx);
+  const PipeType *PipeTy = cast<PipeType>(Arg0->getType());
----------------
Argidx -> ArgIdx

================
Comment at: lib/Sema/SemaChecking.cpp:316
@@ +315,3 @@
+  // the type of pipe element should also be the same.
+  if (!Argidx->getType()->isPointerType() || !ArgTy ||
+      EltTy != ArgTy->getPointeeType().getTypePtr()) {
----------------
Aren't first and second checking the same thing?

================
Comment at: lib/Sema/SemaChecking.cpp:378
@@ +377,3 @@
+  }
+  call->setType(S.getASTContext().IntTy);
+  return false;
----------------
Do we need setArg as well? Not sure though...

================
Comment at: lib/Sema/SemaChecking.cpp:445
@@ +444,3 @@
+  }
+
+  return false;
----------------
No call->setType() here?

================
Comment at: lib/Sema/SemaChecking.cpp:765
@@ +764,3 @@
+  case Builtin::BIsub_group_reserve_write_pipe:
+    // Since those two functions are declared with var args, therefore we need
+    // a semantic check for the argument.
----------------
What two functions do you refer to?

I don't think we have var args here i .e the number of arguments is fixed to 2 and not a variable number!

I think the checking is due to generic argument types to be passed.


http://reviews.llvm.org/D15914