[cfe-commits] [PATCH] OpenCL event type

[Joey Gouly]

Hi Guy,

+    if (R->isEventT()) {
+      if (S->getParent() == NULL) {
Either "== 0", or "!S->getParent()" would be better for the Clang coding
style.

Your diff had something weird with lib/Sema/SemaDeclCXX.cpp, could have been
a Windows line ending issue, but please don't commit that.

Otherwise, it looks good to me.

Thanks,
Joey

-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu
[mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Benyei, Guy
Sent: 07 January 2013 20:08
To: Anton Lokhmotov; gribozavr at gmail.com
Cc: cfe-commits at cs.uiuc.edu
Subject: Re: [cfe-commits] [PATCH] OpenCL event type

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.