bader accepted this revision.
bader added a comment.
LGTM.
A few style nitpicks.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2180-2181
@@ +2179,4 @@
+ }
+ // Could have events and/or vaargs.
+ if (NumArgs >= 5) {
+ if (E->getArg(3)->getType()->isBlockPointerType()) {
----------------
[Style] Minor suggestion to consider.
To avoid indentation of almost 100 lines of code inside if statement this can
be implemented as:
if (NumArgs < 5) llvm_unreachable("Unhandled enqueue_kernel signature");
There is no else branch for that condition anyway.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2189-2190
@@ +2188,4 @@
+ // express to the runtime the number of variadic arguments.
+ std::vector<llvm::Value *> Args{Queue, Flags, Range, Block,
+ ConstantInt::get(IntTy, NumArgs - 4)};
+ std::vector<llvm::Type *> ArgTys = {QueueTy, IntTy, RangeTy, Int8PtrTy,
----------------
[Style] Use an equals before the open curly brace.
http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2195
@@ +2194,3 @@
+ // Add the variadics.
+ for (unsigned i = 4; i < NumArgs; ++i) {
+ llvm::Value *ArgSize = EmitScalarExpr(E->getArg(i));
----------------
[Style] CamelCase: i -> I
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2238-2239
@@ +2237,4 @@
+ EventPtrAS4Ty, EventPtrAS5Ty, Int8PtrTy};
+ std::vector<llvm::Value *> Args{Queue, Flags, Range, NumEvents,
+ EventList, ClkEvent, Block};
+
----------------
[Style] Use an equals before the open curly brace.
http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2249
@@ +2248,3 @@
+ llvm::ArrayRef<llvm::Value *>(Args)));
+ } else {
+ // Has event info and variadics
----------------
[Style] 'else' here is unnecessary:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2257
@@ +2256,3 @@
+ // Add the variadics.
+ for (unsigned i = 7; i < NumArgs; ++i) {
+ llvm::Value *ArgSize = EmitScalarExpr(E->getArg(i));
----------------
[Style] CamelCase: i -> I
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
================
Comment at: lib/Sema/SemaChecking.cpp:124-126
@@ +123,5 @@
+ diag::err_opencl_enqueue_kernel_expected_type) << "block";
+ return true;
+ } else
+ return checkBlockArgs(S, BlockArg);
+}
----------------
[Style] 'else' here is unnecessary:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lib/Sema/SemaChecking.cpp:132
@@ +131,3 @@
+ bool IllegalParams = false;
+ for (unsigned i = Start; i <= End; ++i) {
+ QualType Ty = TheCall->getArg(i)->getType();
----------------
[Style] CamelCase: i -> I
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
================
Comment at: lib/Sema/SemaChecking.cpp:145
@@ +144,3 @@
+}
+
+/// OpenCL v2.0, s6.13.17.1 - Check that sizes are provided for all
----------------
Anastasia wrote:
> I think the problem is that in C99 there are implicit casts among integer
> types, therefore making this check more restrictive would imply the cast has
> to be done explicitly instead.
>
> Thus, this would have to be modified as follows:
>
> enqueue_kernel(..., 64); -> enqueue_kernel(..., (uint)64); or
> enqueue_kernel(..., 64U);
>
> which in my opinion is undesirable and also a bit unexpected because by
> analogy to C99 you can compile the following successfully:
>
> void g(unsigned);
>
> void f() {
> char i;
> g(i);
> }
>
> I have added a check for a size however not to be larger than 32 bits and
> handled type cast in CodeGen. The test cases are added too.
>
> What's your opinion about it?
I'm OK with allowing implicit conversions as long as there is a way to enforce
strict check for unsigned 32-bit integer here (e.g. with -Wconversion or in
pedatic mode).
Can we check that mode?
================
Comment at: lib/Sema/SemaChecking.cpp:238-239
@@ +237,4 @@
+ << "block";
+ return true;
+ } else { // we have a block type, check the prototype
+ const BlockPointerType *BPT =
----------------
[Style] 'else' here is unnecessary:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lib/Sema/SemaChecking.cpp:250
@@ +249,3 @@
+ return false;
+ } else {
+ // we can have block + varargs.
----------------
[Style] 'else' here is unnecessary:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lib/Sema/SemaChecking.cpp:297
@@ +296,3 @@
+ return false;
+ else // check if varargs are correct types.
+ return checkEnqueueVariadicArgs(S, TheCall, Arg6, 7);
----------------
[Style] 'else' here is unnecessary:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
http://reviews.llvm.org/D20249
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
