[cfe-commits] [PATCH] OpenCL event type

Thu Jan 17 17:10:24 PST 2013

Guy,

I am fine with this being a pointer. The patch seems ok to me.

-Tanya

On Jan 16, 2013, at 4:37 AM, "Benyei, Guy" <guy.benyei at intel.com> wrote:

> Tanya,
> Do you have additional comments on this?
> 
> Thanks
>   Guy
> 
> 
> -----Original Message-----
> From: Benyei, Guy 
> Sent: Wednesday, January 09, 2013 16:40
> To: Benyei, Guy; Tanya Lattner
> Cc: cfe-commits at cs.uiuc.edu; Anton.Lokhmotov at arm.com
> Subject: RE: [cfe-commits] [PATCH] OpenCL event type
> 
> 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: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Benyei, Guy
> Sent: Tuesday, January 08, 2013 23:08
> To: Tanya Lattner
> Cc: cfe-commits at cs.uiuc.edu; Anton.Lokhmotov at arm.com
> 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:lattner at apple.com]
> Sent: Tuesday, January 08, 2013 22:43
> To: Benyei, Guy
> Cc: Anton.Lokhmotov at arm.com; gribozavr at gmail.com; cfe-commits at cs.uiuc.edu
> 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" <guy.benyei at intel.com> wrote:
> 
>> Attached a patch with fixes.
>> 
>> Please review.
>> 
>> Thanks
>>  Guy
>> 
>> -----Original Message-----
>> From: Anton Lokhmotov [mailto:Anton.Lokhmotov at arm.com]
>> Sent: Thursday, December 27, 2012 21:02
>> To: Benyei, Guy; gribozavr at gmail.com
>> Cc: cfe-commits at cs.uiuc.edu
>> 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 <guy.benyei at intel.com> 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
>> 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.
> 
> 
> _______________________________________________
> 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.
> 