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