Tanya,
After short discussion, here are the pros for using pointer to opaque type:
1. Makes event_t variables recognizable in LLVM IR - makes it easy for OpenCL
pass to do OpenCL/vendor specific stuff while taking these variables in
account, and makes it possible to create verifier pass that checks the event_t
restrictions.
2. Avoids doing possibly illegal operations by optimization passes.
3. Flexible solution, enabling every vendor to implement his own event type by
simply implementing the opaque type that could hold any synchronization object
or information.
So far we didn't find any cons.
Could you please provide an explanation why size_t would be a better solution?
Thanks
Guy
-----Original Message-----
From: [email protected] [mailto:[email protected]]
On Behalf Of Benyei, Guy
Sent: Tuesday, January 08, 2013 23:08
To: Tanya Lattner
Cc: [email protected]; [email protected]
Subject: Re: [cfe-commits] [PATCH] OpenCL event type
Tanya,
The OpenCL spec doesn't define the actual size or structure of event_t. The
usage of event_t is also very limited: it can be defined as function local
variable or passed to a non-kernel function as argument. It cannot be
initialized, casted to or from other types, assigned, etc... The spec also says
that functions expecting an event_t argument might take a zero instead. This
behavior barely resembles the size_t type's behavior.
Modeling event_t as pointer to opaque struct makes sense, since it can hide any
vendor specific synchronization structure, and the opaque type could be defined
by the vendor, or the pointer could be used as handle or bitcasted to size_t.
Defining event_t as size_t seems to be a less flexible implementation, but
again, the CGOpenCLRuntime was meant to be overloaded by the different vendors
to implement their own OpenCL specifics: local variable handling or OpenCL
specific types.
Thanks
Guy
-----Original Message-----
From: Tanya Lattner [mailto:[email protected]]
Sent: Tuesday, January 08, 2013 22:43
To: Benyei, Guy
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [cfe-commits] [PATCH] OpenCL event type
Guy,
Sorry for the delay as I have been on vacation out of the country.
Why are you choosing to make event_t a pointer to opaque struct?
We have event_t implemented as size_t and I would like to discuss the pros and
cons on this issue.
-Tanya
On Jan 7, 2013, at 12:07 PM, "Benyei, Guy" <[email protected]> wrote:
> Attached a patch with fixes.
>
> Please review.
>
> Thanks
> Guy
>
> -----Original Message-----
> From: Anton Lokhmotov [mailto:[email protected]]
> Sent: Thursday, December 27, 2012 21:02
> To: Benyei, Guy; [email protected]
> Cc: [email protected]
> Subject: Re: [PATCH] OpenCL event type
>
> + // Convert a NULL value for OpenCL event_t initialization
> Please add '.'.
>
> + CK_NullToOCLEvent
> In fact, the spec uses 'zero' instead of 'null', e.g.: "If event argument is
> non-zero, the event object supplied in event argument will be returned."
> While the NULL macro is commonly defined to be 0, the spec does not mandate
> this, as far as I remember. Hence, I'd recommend changing this to
> CK_ZeroToOCLEvent.
>
> + /// \brief Passing NULL to a function where OpenCL event_t is expected.
> + SK_OCLNULLEvent
> ...
> + /// \brief Add a step to initialize an OpenCL event_t from a NULL
> + /// constant.
> + void AddOCLNullEventStep(QualType T);
> Similarly here.
>
>
> + case BuiltinType::OCLEvent:
> + return getOrCreateStructPtrType("opencl_event_t",
> + OCLEventDITy);
> I suggested adding address spaces explicitly before: global for the image
> types, private for the event type, e.g.:
>
> * unsigned AddrSpace =
> CGM.getContext().getTargetAddressSpace(LangAS::opencl_private);
> * return getOrCreateStructPtrType("opencl_event_t",
> * OCLEventDITy, AddrSpace);}
>
> Could you please fix it for the image types too (fine if in a different
> patch)?
>
>
>
>> On Mon, Dec 24, 2012 at 9:06 PM, Benyei, Guy <[email protected]> wrote:
>>> All these diagnostics are quotes from the OpenCL 1.2 spec, section 6.9.
> I
>> can change them to make them clearer, of course, but isn't it the
>> best solution to quote the spec?
>>
>> If these terms are used consistently in OpenCL spec, there might be
>> value in using them, too.
>
> The spec may be unnecessarily verbose here. As there are only four address
> space qualifiers, the message:
>
> + "the event type cannot be used with the __local, __constant and
> + __global
> "
> + "address space qualifiers">;
>
> can be succinctly rephrased as:
>
> "the event_t type can only be used with the private address space qualifier".
>
> (Or even: "only the private address space qualifier can be used with
> the event_t type".)
>
>
> + if (R.getAddressSpace() == LangAS::opencl_local ||
> + R.getAddressSpace() == LangAS::opencl_global ||
> + R.getAddressSpace() == LangAS::opencl_constant) {
> This of course can be simplified too:
> * if (R.getAddressSpace() != LangAS::opencl_private) {
>
>
> For consistency, please also change:
>
> + "arguments to kernel functions in a program cannot be declared to be of "
> + "type event_t">;
>
> to
>
> "the event_t type cannot be used to declare a kernel function argument".
>
>
>> But most other diagnostics have 'member'
> The spec uses the term 'field', but 'member' is also fine by me.
>
>> and 'file scope' in them
> In fact, OpenCL does not use the concept of 'file': programs are passed as
> strings of characters. Hence, using the term 'program scope' is correct, in
> my opinion.
>
> + // OpenCL 1.2 spec, s6.8 r:
> OpenCL v1.2, s6.9.r (note '9', not '8', since you refer to v1.2).
>
> +// OpenCL 1.2 spec, p6.12.10
> OpenCL v1.2, s6.12.10 (note 's', not 'p')
>
> + if (TryOCLNULLEventInitialization(S, *this, DestType, Initializer)) {
> + return;
> + }
> Unnecessary braces?
>
> + foo(5); // expected-error {{passing 'int' to parameter of
> + incompatible
> type 'event_t'}}
> The problem is not that it is 'int'; the problem is that it is non-zero.
>
> Thanks,
> Anton.
>
>
>
> ---------------------------------------------------------------------
> 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_events4.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.
_______________________________________________
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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
