[PATCH] Error on illegal OpenCL kernel argument types

[Richard Smith]

================
Comment at: lib/Sema/SemaDecl.cpp:5960
@@ +5959,3 @@
+
+void Sema::checkIsValidOpenCLKernelArgument(Declarator &D, ParmVarDecl *Param) {
+  QualType PT = Param->getType();
----------------
This doesn't need to be a Sema member; a static non-member function would be preferred.

================
Comment at: lib/Sema/SemaDecl.cpp:5917
@@ -5916,1 +5916,3 @@
 
+static bool isValidOpenCLKernelArgumentType(QualType PT) {
+  // OpenCL v1.2 s6.9.k:
----------------
"...ParameterType" not "...ArgumentType".

================
Comment at: lib/Sema/SemaDecl.cpp:5919
@@ +5918,3 @@
+  // OpenCL v1.2 s6.9.k:
+  // Arguments to kernel functions in a program cannot be declared with the
+  // built-in scalar types bool, half, size_t, ptrdiff_t, intptr_t, and
----------------
I assume this part of the OpenCL spec is defective and it means "Parameters" not "Arguments" here?

================
Comment at: lib/Sema/SemaDecl.cpp:5927-5937
@@ +5926,13 @@
+
+    if (const TypedefType *Typedef = dyn_cast<TypedefType>(PT)) {
+      const IdentifierInfo *Identifier = Typedef->getDecl()->getIdentifier();
+      StringRef Name = Identifier->getName();
+
+      if (Name == "size_t" ||
+          Name == "ptrdiff_t" ||
+          Name == "intptr_t" ||
+          Name == "uintptr_t") {
+        return false;
+      }
+    }
+  }
----------------
OpenCL 6.1.1 seems to imply that size_t, ptrdiff_t, intptr_t and uintptr_t are distinct built-in types in OpenCL, rather than being typedefs for some standard integer type, and it's the underlying types that we should be checking for here, not the typedefs. "typedef size_t my_size_t;" should also be prohibited here (as should, presumably, a typedef for __typeof(sizeof(0))).

However, the OpenCL spec is unclear, so I'm somewhat guessing... :(

================
Comment at: lib/Sema/SemaDecl.cpp:5952-5958
@@ +5951,9 @@
+
+static const RecordType *getAsStructOrUnionType(QualType QT) {
+  const RecordType *RT = QT->getAsStructureType();
+  if (!RT)
+    RT = QT->getAsUnionType();
+
+  return RT;
+}
+
----------------
This is just QT->getAs<RecordType>().

================
Comment at: lib/Sema/SemaDecl.cpp:5966-5969
@@ +5965,6 @@
+  // pointer to a pointer type.
+  if (PT->isPointerType() && PT->getPointeeType()->isPointerType()) {
+    Diag(Param->getLocation(), diag::err_opencl_ptrptr_kernel_arg);
+    D.setInvalidType();
+  }
+
----------------
It would seem cleaner to fold both this pointer check and the one below into isValidOpenCLKernelArgumentType. You can return an enum value from there to indicate the flavor of problem, and fold together the diagnostics with a %select. You can also pass a value into this function to indicate whether you're checking a direct argument (where only a pointer-to-pointer is blocked) or a field of an argument (where any pointer is blocked).

================
Comment at: lib/Sema/SemaDecl.cpp:5989-5990
@@ +5988,4 @@
+    const RecordDecl *RD = RT->getDecl();
+    for (RecordDecl::field_iterator I = RD->field_begin(),
+           E = RD->field_end(); I != E; ++I) {
+      const FieldDecl *FD = *I;
----------------
Have you considered caching the result of this check? Or at least, storing a set of the types for which you've performed the check -- we don't really need to diagnose the same type more than once.

================
Comment at: lib/Sema/SemaDecl.cpp:6000-6002
@@ +5999,5 @@
+      if (!isValidOpenCLKernelArgumentType(QT)) {
+        // TODO: A more specific warning about which struct members forbid this
+        // would be useful
+        Diag(Param->getLocation(), diag::err_bad_kernel_arg_type) << PT;
+        D.setInvalidType();
----------------
Maybe reformulate the queue as a recursive walk, and produce notes on the way back up after diagnosing a problem?


http://llvm-reviews.chandlerc.com/D1052