[cfe-commits] [PATCH] OpenCL event type

[Tanya Lattner]

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