[PATCH] D141008: [Clang][SPIR-V] Emit target extension types for OpenCL types on SPIR-V.

[Anastasia Stulova via Phabricator via llvm-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2260
 
-    return CGF.CGM.getNullPointer(cast<llvm::PointerType>(ConvertType(DestTy)),
-                              DestTy);
+    // The type may be a target extension type instead of a pointer type
+    // (e.g., OpenCL types mapped for SPIR-V). In the former case, emit a
----------------
Ok, yet this looks strange to me... do you have an example that hits this code?

At some point we added `CK_ZeroToOCLOpaqueType`	so I wonder if we should be using this instead of `CK_NullToPointer` here i.e. ideally clang should not assume too early how type are mapped into target specific representation.



================
Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:200
+
+  // Choose the dimension of the image--this corresponds to the Dim parameter,
+  // so (e.g.) a 2D image has value 1, not 2.
----------------
Any reason for this? Can we create a `constexpr` map or enum type that would contain those numbers instead of using hard coded ones scattered around?


================
Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:226
+
+llvm::Type *CGSpirVOpenCLRuntime::convertOpenCLSpecificType(const Type *T) {
+  assert(T->isOpenCLSpecificType() && "Not an OpenCL specific type!");
----------------
This looks good in general but we keep CodeGen hierarchy target agnostic so the way to add specifics of an individual target is to extend `TargetCodeGenInfo` adding a new target hook member function for example it could be `getOpenCLSpecificType` or something like that. Then you can add SPIR-V specific logic into either `CommonSPIRTargetCodeGenInfo` or `SPIRVTargetCodeGenInfo` subclass. I would recommend to look at 
 `createEnqueuedBlockKernel` for inspiration https://clang.llvm.org/doxygen/CodeGen_2TargetInfo_8cpp_source.html#l12407. 




================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:239
+  // extension types.
+  if (getTriple().isSPIRV() || getTriple().isSPIR())
+    OpenCLRuntime.reset(new CGSpirVOpenCLRuntime(*this));
----------------
Do we want to change old SPIR representation or keep it as is? It seems that SPIR spec defined them as LLVM's opaque pointer types... but I appreciate that for maintenance purposes it's easier to keep those in sync.


================
Comment at: llvm/docs/SPIRVUsage.rst:103
+
+All integer arguments take the same value as they do in the SPIR-V type name.
+For example, the OpenCL type ``image2d_depth_ro_t`` would be represented in
----------------
svenvh wrote:
> 
Any reference to the document/section explaining these instructions would be useful here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141008/new/

https://reviews.llvm.org/D141008