[cfe-commits] [PATCH] OpenCL sampler type
Benyei, Guy
guy.benyei at intel.com
Sun Jun 24 01:19:32 PDT 2012
Hi,
The patch looks quite good to me, but I wonder if generating i32 to represent sampler_t is right for everyone. It's right, that samplers should be initialized with 32 bit integers, but that's it. Samplers are opaque types; they may have an entirely different behavior than i32. I agree, that using i32 is a quite straight-forward implementation, but is it good for all the OpenCL implementers?
Thanks
Guy
-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Tanya Lattner
Sent: Friday, June 22, 2012 21:00
To: Douglas Gregor
Cc: cfe-commits at cs.uiuc.edu
Subject: Re: [cfe-commits] [PATCH] OpenCL sampler type
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
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
---------------------------------------------------------------------
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.
More information about the cfe-commits
mailing list