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

[Alexey Bader via cfe-commits]

bader added a comment.

Hi Anastasia,

Sorry for the delay.
I have just a few comments.

Thanks,
Alexey


================
Comment at: lib/Sema/SemaChecking.cpp:95
@@ +94,3 @@
+      IllegalParams = true;
+      break;
+    }
----------------
It looks like this function will report only first invalid parameter. Can we report all of them by moving code from lines 109-120 inside the loop?

================
Comment at: lib/Sema/SemaChecking.cpp:145
@@ +144,3 @@
+  for (unsigned i = Start; i < End; ++i)
+    if (!TheCall->getArg(i)->getType().getTypePtr()->isIntegerType())
+      return false;
----------------
If I understand it correctly the intention is to convert this integer type to i32. Am I right?
By the spec it must be unsigned 32-bit integer, not just any integer type.


================
Comment at: lib/Sema/SemaChecking.cpp:201
@@ +200,3 @@
+///                    clk_event_t *event_ret,
+///                    void (^block)(void*, ...),
+///                    uint size0, ...)
----------------
block arguments must be pointers to the local memory (the same as previous declaration).

================
Comment at: lib/Sema/SemaChecking.cpp:249
@@ +248,3 @@
+    } else
+      return checkBlockArgs(S, Arg3);
+  } else if (NumArgs >= 5) {
----------------
Could you clarify that code path?
My understanding is that if NumArgs ==4, clang should except only block with empty parameter list. Am I right?

   /// int enqueue_kernel(queue_t queue,
   ///                    kernel_enqueue_flags_t flags,
   ///                    const ndrange_t ndrange,
   ///                    void (^block)(void))

================
Comment at: lib/Sema/SemaChecking.cpp:250
@@ +249,3 @@
+      return checkBlockArgs(S, Arg3);
+  } else if (NumArgs >= 5) {
+    // we can have block + varargs.
----------------
This check is redundant.
It's known to be always true at this point.

================
Comment at: lib/Sema/SemaChecking.cpp:297
@@ +296,3 @@
+        return false;
+      else if (NumArgs >= 7) // check if varargs are correct types.
+        return checkEnqueueVariadicArgs(S, TheCall, Arg6, 7);
----------------
This check is redundant. This condition is known to be true at line 250.

================
Comment at: lib/Sema/SemaChecking.cpp:302
@@ +301,3 @@
+
+  // None of the specific case has been detected, give generic error
+  S.Diag(TheCall->getLocStart(),
----------------
Shouldn't by default we return false and report an error only if checks above find inconsistency?
I expect code to be much simpler with this approach.


http://reviews.llvm.org/D20249