[cfe-commits] [PATCH] OpenCL event type

[Douglas Gregor]

On Dec 20, 2012, at 3:13 AM, "Benyei, Guy" <guy.benyei at intel.com> wrote:

> Hi Dmitri,
> Thanks for the review.
> 
> Attached the updated patch. Please review.

Index: include/clang/Serialization/ASTBitCodes.h
===================================================================
--- include/clang/Serialization/ASTBitCodes.h	(revision 170542)
+++ include/clang/Serialization/ASTBitCodes.h	(working copy)
@@ -713,7 +713,8 @@
       /// \brief OpenCL 2d image array type.
       PREDEF_TYPE_IMAGE2D_ARR_ID = 42,
       /// \brief OpenCL 3d image type.
-      PREDEF_TYPE_IMAGE3D_ID    = 43
+      PREDEF_TYPE_IMAGE3D_ID    = 43,
+      PREDEF_TYPE_EVENT_ID      = 45
     };

Why the skip? Because you expect an PREDEF_TYPE_IMAGE3D_ARR_ID that seems like it should exist? The AST format isn't actually stable anyway, so it's fine to just use 44 and we'll end up renumbering later on.

Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp	(revision 170542)
+++ lib/Sema/SemaExpr.cpp	(working copy)
@@ -2466,10 +2466,22 @@
         valueKind = VK_RValue;
         break;
       }
+
+      // In OpenCL, event_t is not l-value.
+      if (getLangOpts().OpenCL && type->isEventT()) {
+        valueKind = VK_RValue;
+        break;
+      }
       // fallthrough
 
     case Decl::ImplicitParam:
     case Decl::ParmVar: {
+      // In OpenCL, event_t is not l-value.
+      if (getLangOpts().OpenCL && type->isEventT()) {
+        valueKind = VK_RValue;
+        break;
+      }
+
       // These are always l-values.
       valueKind = VK_LValue;
       type = type.getNonReferenceType();

This is really weird, and I see no justification for it in the OpenCL specification. What are you trying to accomplish here?

@@ -4005,6 +4013,18 @@
   return true;
 }
 
+static bool TryOCLNULLEventInitialization(Sema &S,
+                                          InitializationSequence &Sequence,
+                                          QualType DestType,
+                                          Expr *Initializer) {
+  if (!S.getLangOpts().OpenCL || !DestType->isEventT() ||
+    !Initializer->getType()->isIntegralType(S.getASTContext()))
+    return false;
+
+  Sequence.AddOCLNullEventStep(DestType);
+  return true;
+}
+

This doesn't look right. You're saying that it's a null-to-event_t conversion so long as the initializer has integral type, which means this will accept the literal 1 and then fail later. That's not what we'd want if this type were exposed in C++. Why not do the proper integral and null-pointer checks here?

Also, please add comments to this function (with a citation into the OpenCL specification).

@@ -5448,7 +5474,23 @@
       CurInit = S.Owned(Semantic);
       break;
     }
+    case SK_OCLNULLEvent: {
+      assert(Step->Type->isEventT() && 
+             "Event initialization on non event type.");
+
+      if (Entity.getKind() != InitializedEntity::EK_Parameter)
+        S.Diag(Kind.getLocation(), diag::err_event_initialization);

What is this trying to do? I don't see any justification for it in the OpenCL specification.

A number of other semantic checks for event_t seem to be missing, though, such as the checks that event_t isn't used to declare kernel function arguments, program scope variables, or struct/union specifiers.

	- Doug

> Thanks
>    Guy Benyei
> 
> -----Original Message-----
> From: Dmitri Gribenko [mailto:gribozavr at gmail.com] 
> Sent: Wednesday, December 19, 2012 01:36
> To: Benyei, Guy
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [cfe-commits] [PATCH] OpenCL event type
> 
> On Wed, Dec 19, 2012 at 1:19 AM, Benyei, Guy <guy.benyei at intel.com> wrote:
>> The attached patch implements OpenCL event_t as Clang builtin type.
>> 
>> According to the OpenCL spec, this type can’t be initialized, but one 
>> may use NULL instead of it when calling a function.
> 
> +      if (Entity.getKind() != InitializedEntity::EK_Parameter)
> +          S.Diag(Kind.getLocation(), diag::err_event_initialization);
> +      else if (!CurInit.get()->isNullPointerConstant(S.getASTContext(),
> +        Expr::NPC_ValueDependentIsNull))
> +          S.Diag(Kind.getLocation(), diag::err_event_argument_not_null)
> +            << SourceType;
> 
> Indentation is funny here (uses 4 spaces instead of 2, function arguments not aligned to the previous line).
> 
> +  event_t e = 0; // expected-error {{cannot initialize event_t}}
> 
> A better wording, maybe: "initialization of event_t variables is not allowed".  Current wording suggests that there's something to the initialization that can be fixed.
> 
> +  foo(5); // expected-error {{event_t variable or NULL required - got 
> + 'int'}}
> 
> This does not feel like a Clang error message to me.  A better wording might be "passing %1 as event_t function parameter is not allowed; use an event_t variable or NULL" -- but there are certainly better alternatives.
> 
> Dmitri
> 
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> <opencl_events2.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits