[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/CGOpenCLRuntime.cpp:40
 
   switch (cast<BuiltinType>(T)->getKind()) {
   default:
----------------
I am not sure if this can be called a generic implementation as it has always been a workaround to compensate for absence of representation in LLVM IR... we should probably move such code into default definition of `TargetInfo::getOpenCLType`...


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10986
+  // for more details).
+  SmallVector<unsigned, 7> IntParams = {0, 0, 0, 0, 0, 0};
+
----------------
I think the list initialization doesn't do what you are trying to achieve with `SmallVector` as it appends the elements. You should probably just be using constructor with size i.e. IntParams(6)?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10989
+  // Choose the dimension of the image--this corresponds to the Dim parameter
+  // (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_dim), or
+  // the instances of Dim in llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td.
----------------
Is this URL likely to stay the same? We normally just add a spec section number e.g.

```
SPIR-V spec v1.6 rev2 s3.8.
```


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10990
+  // (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_dim), or
+  // the instances of Dim in llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td.
+  if (OpenCLName.startswith("image2d"))
----------------
We should avoid adding file paths and names as they can change without updating the comment.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10992
+  if (OpenCLName.startswith("image2d"))
+    IntParams[0] = 1; // 1D
+  else if (OpenCLName.startswith("image3d"))
----------------
Ok, is the order of parameters defined or documented somewhere? Would it make sense to create some kind of a local enum map containing the indices and use enum members instead of constants to improve readability/maintenance?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:11015
+
+llvm::Type *CommonSPIRTargetCodeGenInfo::getOpenCLType(CodeGenModule &CGM,
+                                                       const Type *Ty) const {
----------------
Ok, so the old SPIR format will change too!

I don't have any objections but want to make sure it's not accidental.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141008