[cfe-dev] [OpenCL patch] sampler_t as a builtin type

[Benyei, Guy]

Hi Tanya,

My team and I began reviewing your patches. I'll send my remarks about the other patches soon.

I think it's a great idea to add sampler_t as a built-in type in clang.
On the other hand, if sampler_t is a built-in type, clang should check for right usage of it. There are very clear restrictions about the sampler_t type in the OpenCL spec (6.11.13.1):

Samplers cannot be declared as arrays, pointers, or be used as the type for local variables inside a
function or as the return value of a function defined in a program. Samplers cannot be passed as
arguments to user defined functions. A sampler argument to a __kernel function cannot be
modified.

--- test/Parser/samplerTy.cl        (revision 0)
+++ test/Parser/samplerTy.cl     (revision 0)
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+sampler_t X = 6;
+sampler_t test_vec_step() {
+  return X;
+}
\ No newline at end of file

According the OpenCL spec, the correct usage should be:
const sampler_t X = 6;

Returning sampler type is prohibited. I think the right test should try to do stuff like this, and check if there is an error message. You should also check that the user can't do any arithmetic operations on sampler_t.

===================================================================
--- include/clang/Basic/Specifiers.h         (revision 127366)
+++ include/clang/Basic/Specifiers.h      (working copy)
@@ -55,7 +55,8 @@
     TST_typeofExpr,
     TST_decltype,     // C++0x decltype
    TST_auto,         // C++0x auto
-    TST_error         // erroneous type
+    TST_error,         // erroneous type
+    TST_sampler       // OpenCL sampler_t
   };

Shouldn't we preserve TST_error as the last value in this enum? It looks to me like it was put there for a reason.

===================================================================
--- include/clang/AST/Type.h     (revision 127366)
+++ include/clang/AST/Type.h  (working copy)
@@ -1458,6 +1459,7 @@
     Char16,   // This is 'char16_t' for C++.
     Char32,   // This is 'char32_t' for C++.
     UShort,
+    Sampler,  // OpenCL Sampler type, 'sampler_t', (6.11.8).
     UInt,
     ULong,
     ULongLong,

I'm not sure the Sampler enum should be right in the middle of this enumeration.

===================================================================
--- lib/AST/ItaniumMangle.cpp  (revision 127366)
+++ lib/AST/ItaniumMangle.cpp               (working copy)
@@ -1330,6 +1330,7 @@
   case BuiltinType::ObjCId: Out << "11objc_object"; break;
   case BuiltinType::ObjCClass: Out << "10objc_class"; break;
   case BuiltinType::ObjCSel: Out << "13objc_selector"; break;
+  case BuiltinType::Sampler: Out << "uSampler"; break;
   }
}

Is this a standard Itanium mangling for sampler types? If not, then it should be an assert.

===================================================================
--- lib/CodeGen/CGRTTI.cpp       (revision 127366)
+++ lib/CodeGen/CGRTTI.cpp    (working copy)
@@ -191,6 +191,7 @@
     case BuiltinType::Char32:
     case BuiltinType::Int128:
     case BuiltinType::UInt128:
+    case BuiltinType::Sampler:
       return true;

This part of clang is for C++ - I don't think samplers are relevant here.


Thanks

      Guy Benyei

      Intel

      SSG - MGP OpenCL Development Center




[cid:image001.png at 01CBE4F2.C84D2A70]

From: cfe-dev-bounces at cs.uiuc.edu [mailto:cfe-dev-bounces at cs.uiuc.edu] On Behalf Of Tanya Lattner
Sent: Thursday, March 17, 2011 02:04
To: cfe-dev at cs.uiuc.edu Developers
Subject: Re: [cfe-dev] [OpenCL patch] sampler_t as a builtin type

ping.. any comments?

On Mar 9, 2011, at 4:54 PM, Tanya Lattner wrote:


This patch adds the OpenCL sampler type as a built in type. The sampler type is defined as follows:

The sampler_t type is a 32-bit unsigned int constant and is interpreted as a bit-field that specifies the following properties:
Addressing Mode, Filter Mode, Normalized Coordinates. These properties control how elements of an image object are read by read_image{f|i|ui}.

We made this a built in type because we needed a way to distinguish it from normal integers when checking for special properties of the sampler type (ie. global samplers are not required to be in the constant address space while global integers are).

A simple test case is included.

I was not sure what to do about these warnings since I'm not familiar with this code. If you have some suggestions, please let me know:

CIndexUSRs.cpp: In member function 'void<unnamed>::USRGenerator::VisitType(clang::QualType)':
CIndexUSRs.cpp:526: warning: enumeration value 'Sampler' not handled in switch

CIndex.cpp: In member function 'bool<unnamed>::CursorVisitor::VisitBuiltinTypeLoc(clang::BuiltinTypeLoc)':
CIndex.cpp:1356: warning: enumeration value 'Sampler' not handled in switch

For your review:

<samplerTy.patch>


Thanks,
Tanya

_______________________________________________
cfe-dev mailing list
cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

---------------------------------------------------------------------
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110317/555cbbdf/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 24800 bytes
Desc: image001.png
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110317/555cbbdf/attachment.png>