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