[PATCH] D33989: [OpenCL] Allow targets to select address space per type

[Yaxun Liu via Phabricator via cfe-commits]

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