[cfe-commits] [PATCH] OpenCL event type

Tue Jan 8 13:08:04 PST 2013

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.