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.
-Tanya
> Index: include/clang/AST/TypeLoc.h
> ===================================================================
> --- include/clang/AST/TypeLoc.h (revision 158757)
> +++ include/clang/AST/TypeLoc.h (working copy)
> @@ -1360,6 +1360,12 @@
> ComplexType> {
> };
>
> +// FIXME: location of the OpenCL sampler type.
> +class OpenCLSamplerTypeLoc : public
> InheritingConcreteTypeLoc<TypeSpecTypeLoc,
> + OpenCLSamplerTypeLoc,
> + OpenCLSamplerType> {
> +};
> +
>
> TypeSpecTypeLoc has the right storage already for the sampler_t type, so
> there really isn't anything to FIXME here. (Or, you'd get this for free if
> sampler_t was jut another builtin type).
>
> Index: lib/Sema/TreeTransform.h
> ===================================================================
> --- lib/Sema/TreeTransform.h (revision 158757)
> +++ lib/Sema/TreeTransform.h (working copy)
> @@ -4524,6 +4524,13 @@
> }
>
> template<typename Derived>
> +QualType TreeTransform<Derived>::TransformOpenCLSamplerType(TypeLocBuilder
> &TLB,
> + OpenCLSamplerTypeLoc TL)
> {
> + QualType Result = TL.getType();
> + return Result;
> +}
> +
>
> You'll actually have to push an OpenCLSamplerTypeLoc into TLB here;
> otherwise, the TypeLocBuilder won't actually have the type-location
> information, and anything based on tree transformation will break with
> sampler_t types. See TransformBuiltinType for an indication of how to fix
> this.
>
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp (revision 158759)
> +++ lib/Sema/SemaDecl.cpp (working copy)
> @@ -4098,6 +4098,33 @@
> // OpenCL __local address space.
> if (R.getAddressSpace() == LangAS::opencl_local)
> SC = SC_OpenCLWorkGroupLocal;
> +
> + if (isa<OpenCLSamplerType>(R.getTypePtr())) {
>
> isa<> doesn't look through sugar. Please use R->getAs<OpenCLSamplerType>() to
> test.
>
> + // The sampler type can be used to declare a variable only
> + // in the scope of a kernel function or in the program scope.
> + if (DC->isFunctionOrMethod()) {
> + FunctionDecl *FD = dyn_cast<FunctionDecl>(DC);
> + if (FD->hasAttr<OpenCLKernelAttr>()) {
> + valid = true;
> + }
> + } else if (DC->isTranslationUnit()) {
> + valid = true;
> + }
>
> I know OpenCL doesn't have namespaces or ObjC methods, but I think this code
> should still be robust against those possibilities. In the
> hasAttr<OpenCLKernelAttr>() check, please check that 'FD' is non-NULL first
> (to catch the ObjC method case). I also suggest replacing the
> DC->isTranslationUnit() check with DC->isFileContext() (to handle namespaces).
>
> + if (R.hasQualifiers()
> + && !((R.getAddressSpace() == LangAS::opencl_constant)
> + || R.isConstQualified())) {
> + Diag(D.getIdentifierLoc(), diag::err_opencl_sampler_qualifier);
> + D.setInvalidType();
> + }
>
> This is a very generic diagnostic, although it's testing for something very
> specific: why not emit something like 'non-const sampler type can only be in
> the constant address space (not address space %0)'?
>
> @@ -9222,6 +9249,14 @@
> }
> }
>
> + if (!InvalidDecl) {
> + // Sampler types cannot be used to declare a field.
> + if (isa<OpenCLSamplerType>(T)) {
> + Diag(Loc, diag::err_opencl_type_field) << II << T;
> + InvalidDecl = true;
> + }
> + }
> +
>
> isa -> getAs
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 158759)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -5626,6 +5626,13 @@
> "kernel functions cannot be declared static">;
> def err_static_function_scope : Error<
> "variables in function scope cannot be declared static">;
> +def err_opencl_sampler_declaration : Error<
> + "declaring sampler variable in this context is not allowed">;
> +def err_opencl_sampler_qualifier : Error<"using invalid qualifier with
> sampler type">;
> +def err_opencl_type_pointer : Error<"%0: declaring pointer to type %1 is not
> allowed">;
> +def err_opencl_type_array : Error<"%0: declaring array of type %1 is not
> allowed">;
> +def err_opencl_type_return : Error<"declaring return value of type %0 is not
> allowed">;
> + def err_opencl_type_field : Error<"%0: declaring field of type %1 is not
> allowed">;
>
> I find the prefixing of '%0:' in these diagnostics to be a bit awkward, for
> two reasons: first, Clang tends to put the names of things in their English
> context when it uses names, e.g., 'variable %0 declared as a pointer to type
> %1'. Moreover, in most of the cases where these diagnostics are emitted,
> there may not be any name, which is going to leave a very awkward ':'. Why
> not drop the names entirely, and follow what other things in SemaType do,
> e.g., "cannot declare field of type %0" or "cannot declare an array of type
> %0"?
>
> @@ -1132,6 +1135,13 @@
> return QualType();
> }
>
> + // Using the sampler types to declare a pointer is not allowed.
> + if (isa<OpenCLSamplerType>(T)) {
> + Diag(Loc, diag::err_opencl_type_pointer)
> + << getPrintableNameForEntity(Entity) << T;
> + return QualType();
> + }
>
> isa -> getAs
>
> @@ -1281,6 +1291,13 @@
> return QualType();
> }
>
> + // Using the image and sampler types to declare an array is not allowed
> + if (isa<OpenCLSamplerType>(T)) {
> + Diag(Loc, diag::err_opencl_type_array)
> + << getPrintableNameForEntity(Entity) << T;
> + return QualType();
> + }
> +
>
> isa -> getAs
>
> @@ -2411,6 +2428,14 @@
> // Only the outermost chunk is marked noexcept, of course.
> EPI.ExceptionSpecType = EST_BasicNoexcept;
> }
> +
> + // Using the sampler types to declare a return value
> + // is not allowed.
> + if (LangOpts.OpenCL) {
> + if (isa<OpenCLSamplerType>(T.getTypePtr())) {
> + S.Diag(DeclType.Loc, diag::err_opencl_type_return) << T;
> + }
> + }
>
> There's no need for the OpenCL check here; also, isa -> getAs
>
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp (revision 158757)
> +++ lib/Sema/SemaExpr.cpp (working copy)
> @@ -5511,6 +5511,12 @@
> LHSType = Context.getCanonicalType(LHSType).getUnqualifiedType();
> RHSType = Context.getCanonicalType(RHSType).getUnqualifiedType();
>
> + // A special case: sampler constructor.
> + if (getLangOpts().OpenCL) {
> + if (isa<OpenCLSamplerType>(LHSType)) {
> + if (RHSType->isIntegerType()) return Compatible;
> + }
> + }
>
> No need for the OpenCL check. isa -> getAs. Also, should there be an
> equivalent standard conversion on the C++ side of the world?
>
> @@ -7466,6 +7472,10 @@
> case Expr::MLV_ConstQualified:
> Diag = diag::err_typecheck_assign_const;
>
> + // OpenCL sampler check, no need to do anything further.
> + if (S.getLangOpts().OpenCL && isa<OpenCLSamplerType>(E->getType()))
> + break;
> +
>
> No need for the OpenCL check. isa -> getAs.
>
> Index: lib/Sema/Sema.cpp
> ===================================================================
> --- lib/Sema/Sema.cpp (revision 158757)
> +++ lib/Sema/Sema.cpp (working copy)
> @@ -273,6 +273,15 @@
> if (ExprTy == TypeTy)
> return Owned(E);
>
> + // A special case: attempting to initialize a sampler.
> + if (getLangOpts().OpenCL) {
> + const OpenCLSamplerType *tp =
> Ty.getTypePtr()->getAs<OpenCLSamplerType>();
> + if (tp != NULL) {
> + // Casting is not required - CodeGen will take care of it
> + return Owned(E);
> + }
> + }
> +
>
> We don't want to do this. It's a conversion in the language, and should be
> represented in the AST. The meta-point here is that CodeGen is supposed to be
> "simple", in the sense that it doesn't have to reason about language
> semantics. The cast kind is there to tell CodeGen what to do.
>
> @@ -2063,6 +2064,10 @@
> mangleType(T->getElementType());
> }
>
> +void CXXNameMangler::mangleType(const OpenCLSamplerType *T) {
> + Out << "uSampler";
> +}
>
> This should be "u7Sampler", because 'Sampler' is not a source-name.
>
> 'twould be nice if the libc++abi demangler knew about this mangling, but
> that's just gravy.
>
> + // OpenCL specific types.
> + if (Ctx.getLangOpts().OpenCL) {
> + const QualType ETy = E->getType();
> + const Type *ETyP = ETy.getTypePtr();
> + if (isa<OpenCLSamplerType>(ETyP)) {
> + return Cl::CM_ConstQualified;
> + }
> + }
>
> This has the isa -> getAs issue.
>
> However, I don't understand the purpose, because it seems like this should
> fall out from the fact that the lvalue will either have 'const' type (which
> is handled above) or is in the constant address space (which seems like a
> missing general case in this code).
>
> Index: lib/CodeGen/CGRTTI.cpp
> ===================================================================
> --- lib/CodeGen/CGRTTI.cpp (revision 158757)
> +++ lib/CodeGen/CGRTTI.cpp (working copy)
> @@ -397,6 +397,9 @@
> #include "clang/AST/TypeNodes.def"
> llvm_unreachable("Non-canonical and dependent types shouldn't get here");
>
> + case Type::OpenCLSampler:
> + llvm_unreachable("OpenCL types shouldn't get here");
> +
> case Type::LValueReference:
> case Type::RValueReference:
> llvm_unreachable("References shouldn't get here");
>
> OpenCL and RTTI, like oil and water.
>
> Index: lib/CodeGen/CGDebugInfo.cpp
> ===================================================================
> --- lib/CodeGen/CGDebugInfo.cpp (revision 158757)
> +++ lib/CodeGen/CGDebugInfo.cpp (working copy)
> @@ -1769,6 +1769,10 @@
> case Type::Atomic:
> return CreateType(cast<AtomicType>(Ty), Unit);
>
> +
> + case Type::OpenCLSampler:
> + llvm_unreachable("unsupported");
> +
>
> Rather than aborting, could we simply use 'Int32' here and add a FIXME if
> anyone ever cares to improve the situation?
>
> Index: lib/CodeGen/CGDecl.cpp
> ===================================================================
> --- lib/CodeGen/CGDecl.cpp (revision 158757)
> +++ lib/CodeGen/CGDecl.cpp (working copy)
> @@ -111,6 +111,12 @@
> case SC_None:
> case SC_Auto:
> case SC_Register:
> + if (isa<OpenCLSamplerType>(D.getType())) {
> + // Emit a global "static" sampler declaration.
> + llvm::GlobalValue::LinkageTypes Linkage =
> + llvm::GlobalValue::InternalLinkage;
> + return EmitStaticVarDecl(D, Linkage);
> + }
> return EmitAutoVarDecl(D);
> case SC_Static: {
> llvm::GlobalValue::LinkageTypes Linkage =
>
> CodeGen shouldn't be making this decision. I suspect there's a more general
> rule at work here that should be handled by Sema and encoded in the AST. For
> example, perhaps local variables in the __constant address space effectively
> have 'static' storage? And that a 'const sampler_t' local variable is
> effectively in the __constant address space?
>
> Also, isa -> getAs.
>
> @@ -184,7 +190,20 @@
> Name = GetStaticDeclName(*this, D, Separator);
>
> llvm::Type *LTy = CGM.getTypes().ConvertTypeForMem(Ty);
> - llvm::GlobalVariable *GV =
> + llvm::GlobalVariable *GV = 0;
> +
> + // Handle OpenCL sampler types.
> + if (getLangOpts().OpenCL) {
> + if (isa<OpenCLSamplerType>(D.getType().getTypePtr())) {
> + GV = new llvm::GlobalVariable(CGM.getModule(), LTy,
> + true, llvm::GlobalValue::ExternalLinkage,
> + 0, Name, 0,
> + D.isThreadSpecified(),
> +
> CGM.getContext().getTargetAddressSpace(Ty));
> + }
> + }
> +
>
> Unnecessary OpenCL check; isa -> getAs.
>
> However, I suspect this code shouldn't be needed at all, if we modeled the
> storage of variables of type sampler_t in the AST (as mentioned above).
> However, I could certainly be missing something.
>
> - Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
