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

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel marked an inline comment as done.
jcranmer-intel 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
----------------
Anastasia wrote:
> 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.
> 
I thought I had a test case that caused this code to be executed it, but after removing it and trying out the entire testsuite with it disabled, it never fired. So these changes are indeed unnecessary.


================
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.
----------------
Anastasia wrote:
> 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?
There are constants in the SPIR-V backend that could conceivably be reused (generated from tablegen), but there's no guarantee that we're being compiled with the LLVM SPIR-V backend enabled, which makes it hard to reuse them.

I can add some more comments to explicitly give the references that specify what the values mean, but I'm not entirely certain it's worth building enums just for this one function to use.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:239
+  // extension types.
+  if (getTriple().isSPIRV() || getTriple().isSPIR())
+    OpenCLRuntime.reset(new CGSpirVOpenCLRuntime(*this));
----------------
Anastasia wrote:
> 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.
>From what I can observe, SPIR and SPIRV seem to be used interchangeably as far as the toolchains are concerned--anything targetting SPIR ends up going through the SPIRV toolchain. This is why I've triggered it for both of the toolchains.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141008