[cfe-dev] [OpenCL patch] sampler_t as a builtin type
Tanya Lattner
lattner at apple.com
Fri Mar 18 10:33:03 PDT 2011
On Mar 17, 2011, at 1:29 PM, Benyei, Guy wrote:
> 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. :)
> 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
>
> Thanks
> Guy Benyei
> Intel
> SSG - MGP OpenCL Development Center
>
>
>
> <image001.png>
>
> 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
> 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/20110318/817306b9/attachment.html>
More information about the cfe-dev
mailing list