[PATCH] D21567: [OpenCL] Generate struct type for sampler_t and function call for the initializer

[Yaxun Liu via cfe-commits]

yaxunl added inline comments.

================
Comment at: include/clang/AST/BuiltinTypes.def:164
@@ +163,3 @@
+// Internal OpenCL sampler initializer type.
+BUILTIN_TYPE(OCLSamplerInit, OCLSamplerInitTy)
+
----------------
Anastasia wrote:
> I can't get why is this necessary to represent initializer as a special Clang type? Could we avoid this additional complexity?
If we don't change the type in AST, we need to translate the sampler type in AST to different types based on whether the entry is a variable or a function argument. This needs some ugly hacking of the codegen.

================
Comment at: include/clang/AST/OperationKinds.def:328
@@ +327,3 @@
+// Convert an integer initializer to an OpenCL sampler initializer.
+CAST_OPERATION(IntToOCLSamplerInitializer)
+
----------------
Anastasia wrote:
> Could we just have only int->sampler conversion? Without having an extra type for initializer?
If we don't insert the sampler initializer to sampler cast in AST, when we want to insert the function call __translate_sampler_initializer in codegen, we have to hacking the generic translation of function call instruction and other instructions which may have a sampler variable as operand to insert the function call, which would be ugly.

Or we could do a post-processing of the llvm module at the end of codegen to insert the function calls, if this approach is acceptable.

================
Comment at: include/clang/Basic/DiagnosticGroups.td:876
@@ +875,3 @@
+// A warning group for warnings about code that clang accepts when
+// compiling OpenCL C/C++ but which is not compatible with the SPIR spec.
+def SpirCompat : DiagGroup<"spir-compat">;
----------------
Anastasia wrote:
> I don't understand the description really? Why not compatible?
e.g. the sampler constant may take some literal value which is not compatible with SPIR/SPIR-V spec. A warning will be emitted. Such warning msg belongs to this category.

================
Comment at: include/clang/Driver/CC1Options.td:690
@@ -689,1 +689,3 @@
   HelpText<"OpenCL only. Allow denormals to be flushed to zero">;
+def cl_sampler_type : Separate<["-"], "cl-sampler-type">,
+  HelpText<"OpenCL only. Specify type of sampler to emit. Valid values: \"opaque\"(default), \"i32\"">;
----------------
Anastasia wrote:
> Any reason to have this flag and support different sampler representations in Clang?
Give user some time for transition.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:52
@@ +51,3 @@
+      return llvm::PointerType::get(llvm::StructType::create(
+                           Ctx, "__sampler"),
+                           CGM.getContext().getTargetAddressSpace(
----------------
Anastasia wrote:
> Could you please keep coherency in type naming i.e. add "opencl." prefix.
We need to be able to implement function 

  __sampler* __translate_sampler_initializer(__sampler_initializer*);

in library, where `__sampler` and `__sampler_initializer` are concrete struct types defined in the library source code. Therefore we cannot have dot in the struct type name.


http://reviews.llvm.org/D21567