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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
