Hi, Thanks for the review, attached the updated patch. Implemented the event_t specific restrictions from the OpenCL 1.2 spec, section 6.9. Changed the AST bit code for event_t - the value 44 was actually reserved for samplers, but that's not an issue too. Defining events as RValue was clearly wrong, so I've removed that part, and also checked for a proper 0 value in TryOCLNULLEventInitialization. I've removed the restriction about initializing event_t variables. The OpenCL spec doesn't specify this, as most of the event_t behavior is defined briefly in the OpenCL built-ins part only.
Please review.
Thanks
Guy Benyei
-----Original Message-----
From: Douglas Gregor [mailto:[email protected]]
Sent: Thursday, December 20, 2012 23:44
To: Benyei, Guy
Cc: Dmitri Gribenko; [email protected]
Subject: Re: [cfe-commits] [PATCH] OpenCL event type
On Dec 20, 2012, at 3:13 AM, "Benyei, Guy" <[email protected]> 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:[email protected]]
> Sent: Wednesday, December 19, 2012 01:36
> To: Benyei, Guy
> Cc: [email protected]
> Subject: Re: [cfe-commits] [PATCH] OpenCL event type
>
> On Wed, Dec 19, 2012 at 1:19 AM, Benyei, Guy <[email protected]> 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 <[email protected]>*/
> ---------------------------------------------------------------------
> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
---------------------------------------------------------------------
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_events3.patch
Description: opencl_events3.patch
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
