[cfe-commits] [PATCH] OpenCL event type
Benyei, Guy
guy.benyei at intel.com
Mon Dec 24 06:07:23 PST 2012
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:dgregor at apple.com]
Sent: Thursday, December 20, 2012 23:44
To: Benyei, Guy
Cc: Dmitri Gribenko; cfe-commits at cs.uiuc.edu
Subject: Re: [cfe-commits] [PATCH] OpenCL event type
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
---------------------------------------------------------------------
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opencl_events3.patch
Type: application/octet-stream
Size: 32647 bytes
Desc: opencl_events3.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121224/5f525bde/attachment.obj>
More information about the cfe-commits
mailing list