[OpenCL patch] sampler_t as a builtin type

Clang Frontend
1

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::USRGenerator::VisitType(clang::QualType)’:
CIndexUSRs.cpp:526: warning: enumeration value ‘Sampler’ not handled in switch

CIndex.cpp: In member function ‘bool::CursorVisitor::VisitBuiltinTypeLoc(clang::BuiltinTypeLoc)’:
CIndex.cpp:1356: warning: enumeration value ‘Sampler’ not handled in switch

For your review:

samplerTy.patch (14.1 KB)

2

ping… any comments?

3

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

image001.png

4

Hi Tanya,

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

Thanks, comments below.

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):

I have another patch that is built on top of this that provides some error messages. I separated patches out to comply with LLVM's policy of incremental development. Whatever error messages I am missing, someone else can add them too. :slight_smile:

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.

Yes, my test case does not meet the OpenCL spec. My fault for being lazy and not writing a good test case for this particular patch.I'll modify my test case.

===================================================================
--- 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.

I don't think it matters, but I can move it.

===================================================================
--- 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.

Where it falls is important I believe as some checks look at the enum order when doing checks for is integer, etc. I'll double check this.

===================================================================
--- 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.

Probably not.

===================================================================
--- 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.

Yes, good point.

-Tanya