On Jun 24, 2012, at 1:19 AM, "Benyei, Guy" <[email protected]> wrote:
> 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? If not, it's a fairly small follow-up patch to make this map to a target-dependent integral type. - Doug > Thanks > Guy > > -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Tanya Lattner > Sent: Friday, June 22, 2012 21:00 > To: Douglas Gregor > Cc: [email protected] > 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 > [email protected] > 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. > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
