[cfe-commits] [PATCH] OpenCL event type

[Anton Lokhmotov]

+  // 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.