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" <[email protected]> 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: [email protected]; [email protected] > 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: [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
