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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Can you link relevant LLVM reviews or documentation and perhaps add some more details into the description?

It's  a bit suboptimal though to emit this form of types as SPIR-V specific.  Ideally Clang should worry as less as possible about target specifics. So it would be better if we emit OpenCL opaque types as opaque in LLVM IR and then let backends expand those to whatever they need them to be.  There isn't technically anything SPIR-V specific in ``target("spirv.Image", void, 1, 1, 0, 0, 0, 0, 0)`` as this form  is just a better representation of the type with more HL information preserved that can either be used or discarded by the optimiser/backend.

My worry is that with the current approach more complexity is being added into the common parts of the compiler. At least it might be better to hide this alternative generation paths into the target implementation itself just like for example we ask target hooks to provide mapping to address spaces. Then we could just have a default path from which targets can derive from...



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2263
+    llvm::Type *LlvmTy = ConvertType(DestTy);
+    if (auto *PointerTy = dyn_cast<llvm::PointerType>(LlvmTy))
+      return CGF.CGM.getNullPointer(PointerTy, DestTy);
----------------
This case at least deserve a comment with explanation. Otherwise I am confused why we end up with a non-pointer type as destination in NullToPointer cast?


================
Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:80
+
+  if (useSPIRVTargetExtType(CGM)) {
+    switch (cast<BuiltinType>(T)->getKind()) {
----------------
Perhaps it's best to split into separate functions and reflect in naming what they correspond to.


================
Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:100
+#define INTEL_SUBGROUP_AVC_TYPE(Name, Id)                                      \
+    case BuiltinType::OCLIntelSubgroupAVC##Id:                                 \
+      return llvm::TargetExtType::get(Ctx, "spirv.Avc" #Id "INTEL");
----------------
Why does this need special handling?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141008