[PATCH] D20249: [OpenCL] Hierarchical/dynamic parallelism - enqueue kernel in OpenCL 2.0

[Alexey Bader via cfe-commits]

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