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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
