[cfe-commits] [PATCH] OpenCL sampler type

[Douglas Gregor]

I've added Anton in case he isn't watching… 

On Jun 22, 2012, at 10:59 AM, Tanya Lattner <lattner at apple.com> wrote:

> 
> On Jun 22, 2012, at 9:21 AM, Douglas Gregor wrote:
> 
>> 
>> On Jun 21, 2012, at 4:51 PM, Tanya Lattner wrote:
>> 
>>> Attached is a patch that adds the OpenCL sampler type to clang. This is a combined patch from the one submitted by Anton Lokhmotov, changes from my tree, and correcting some issues that I saw. 
>>> 
>>> I've purposely separated out the event and image types from the original patch as they should be made in other separate patches.
>>> 
>>> Please review.
>> 
>> Index: include/clang/AST/Type.h
>> ===================================================================
>> --- include/clang/AST/Type.h	(revision 158757)
>> +++ include/clang/AST/Type.h	(working copy)
>> @@ -4533,6 +4533,21 @@
>>  QualType apply(const ASTContext &Context, const Type* T) const;
>> };
>> 
>> +// OpenCL specific types.
>> +class OpenCLSamplerType : public Type {
>> +public:
>> +  OpenCLSamplerType() :  Type(OpenCLSampler, QualType(), false, false,
>> +                              /*VariablyModified=*/false,
>> +                              /*Unexpanded parameter pack=*/false) { }
>> +  bool isSugared() const { return false; }
>> +  QualType desugar() const { return QualType(this, 0); }
>> +  
>> +  static bool classof(const Type *T) {
>> +    return T->getTypeClass() == OpenCLSampler;
>> +  }
>> +  
>> +  static bool classof(const OpenCLSamplerType *) { return true; }
>> +};
>> 
>> // Inline function definitions.
>> 
>> Apologies if I've missed/forgotten the discussion, but why is this a separate type rather than simply another kind of BuiltinType?
>> 
> 
> 
> I'll just comment on this before I go through the rest of your comments. This is from Anton back when we were trying to converge on sampler_t in Clang. I had it as a builtin type but from his perspective it should not be one:
> 
> "Hi Tanya,
> 
> I've invested a lot of time trying to understand the sampler type.  Here's a
> summary.
> 
> The sampler type (sampler_t) is used for sampler objects, either created via
> the OpenCL API and set as a kernel argument, or declared in the program
> source (6.11.13.1).  A sampler value is a 32-bit unsigned integer constant,
> interpreted as a bit-field for the image read properties: addressing mode,
> filtering mode, normalised coordinates.
> 
> == Allowed use ==
> * The sampler type can be used as the type of a function argument.
> * The sampler type can be used to declare a variable in the program scope.
> * The sampler type can be used to declare a variable in a kernel function
> scope.
> 
> == Disallowed use ==
> * The sampler type cannot be used to declare an array of samplers.
> * The sampler type cannot be used to declare a pointer to a sampler.
> * The sampler type cannot be the return type of a function.
> * A sampler function argument cannot be modified.
> * A sampler variable cannot be modified.
> 
> All in all, the sampler type does not behave like a normal integer.  It's
> actually an opaque type which backends can implement in a target-specific
> way.
> 
> I'm going to submit our first patch for the image and sampler types shortly.
> Hope we can do a merge in some way.
> 
> Best regards,
> Anton."
> 
> Some other references:
> My original patch and discussion:
> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/013865.html
> Anton's response:
> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/014118.html
> His patch:
> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/014121.html
> 
> Let me know what you think and if this makes sense to you. 

None of the comments above imply that sampler_t should have its own *Type node. Builtin types aren't simply integers or scalars: they cover all manner of types, from integers and floats to 'void', Objective-C's 'id' and 'SEL', as well as placeholder types like bound member-function pointers, overloaded references, and type-dependent expression types. The fundamental aspect of being a builtin type is that the type is primitive from the language perspective. sampler_t clearly is primitive in OpenCL (for example, because it is a distinct type and is neither a composite of other types nor a tag type), so it belongs as a BuiltinType. This also has the nice property of being easier to deal with in the code base.

Now, there are other questions that need to be answered about sampler_t, regardless of whether it is a BuiltinType or its own *Type node. Is it a scalar type (I imagine it is) ? We need to be sure that Type::isScalarType() does the right there. Is it integral? (Probably not); Type::isIntegralOrEnumerationType() will need to do the right thing.

	- Doug