yaxunl added inline comments.
================ Comment at: cfe/trunk/lib/Basic/TargetInfo.cpp:351 +LangAS::ID TargetInfo::getOpenCLTypeAddrSpace(const Type *T) const { + auto BT = dyn_cast<BuiltinType>(T); ---------------- rsmith wrote: > Anastasia wrote: > > chapuni wrote: > > > Excuse me for old commit, I think it might be layering violation. > > > Could we avoid using AST/Type here? > > One of the problems is that even if we could write another layer of enums > > to map OpenCL specific AST types into the TargetInfo representation it > > doesn't really help creating a layered structure between AST and Basic > > libs. They seem to have a large logical dependence despite not including > > the library headers physically. So modification in AST would require > > modifications in Basic anyway resulting in overhead of changing 2 places > > instead of 1. So I am wondering if there is any better approach here e.g. > > revisiting the library dependencies or what classes they should contain? > I don't know what large logical dependencies you're talking about here. > `Basic` is supposed to be answering questions about lowering that are > independent of our choice of AST model. It should never look at a > `clang::Type` for any reason to do this, and should never need to do so. If > you find it does need to do so, you're asking the `TargetInfo` questions at > the wrong level of abstraction. > > As far as I can see, there's only one question you actually need to ask the > `TargetInfo` here, and that's whether builtin types are in the > `opencl_global` or `opencl_constant` address space. This function has been expanded to include more types and the returned address space is not limited to global/constant. I think we need some enum definition in TargetInfo.h for OpenCL types with target-dependent address spaces, as Anastasia suggested before, e.g. ``` enum OpenCLTypeKindWithTargetDependentAddrSpace { Default, Image, Sampler, //... }; ``` Then define this function with the enum as parameter. Then add a function to ASTContext to map AST types to the enum. ``` Repository: rL LLVM https://reviews.llvm.org/D33989 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits