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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

Can you add missing types to test/CodeGenOpenCL/opencl_types.cl ?



================
Comment at: include/clang/Basic/TargetInfo.h:1034
+  virtual LangAS::ID getOpenCLTypeAddrSpace(BuiltinType::Kind K) const {
+    switch (K) {
+#define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix)                   \
----------------
I think it is a good idea to make this function a central place for all OpenCL types.

Can you also add sampler and pipe type then update CGOpenCLRuntime::getPipeType and CGOpenCLRuntime::getSamplerType to use this function to get address space for sampler and pipe?


================
Comment at: include/clang/Basic/TargetInfo.h:1041
+    default:
+      return LangAS::Default;
+    }
----------------
I think the default (including even_t, clk_event_t, queue_t, reserved_id_t) should be global since these opaque OpenCL objects are pointers to some shared resources. These pointers may be an automatic variable themselves but the memory they point to should be global. Since these objects are dynamically allocated, assuming them in private address space implies runtime needs to maintain a private memory pool for each work item and allocate objects from there. Considering the huge number of work items in typical OpenCL applications, it would be very inefficient to implement these objects in private memory pool. On the other hand, a global memory pool seems much reasonable.

Anastasia/Alexey, any comments on this? Thanks.


================
Comment at: lib/AST/ASTContext.cpp:1775
     case BuiltinType::OCLSampler: {
-      auto AS = getTargetAddressSpace(LangAS::opencl_constant);
+      AS = getTargetAddressSpace(LangAS::opencl_constant);
       Width = Target->getPointerWidth(AS);
----------------
All OpenCL types may be unified as

```
AS = getOpenCLTypeAddrSpace(TK);
Width = Target->getPointerWidth(AS);
Align = Target->getPointerAlign(AS);
```


where `TK = cast<BuiltinType>(T)->getKind();`


================
Comment at: lib/Basic/Targets.cpp:2376
+    default:
+      return LangAS::Default;
+    }
----------------
should return TargetInfo::getOpenCLTypeAddrSpace


https://reviews.llvm.org/D33989