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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: include/clang/AST/BuiltinTypes.def:164
@@ +163,3 @@
+// Internal OpenCL sampler initializer type.
+BUILTIN_TYPE(OCLSamplerInit, OCLSamplerInitTy)
+
----------------
yaxunl wrote:
> 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.
I don't understand why this is needed. Surely, sampler type is just a sampler type independently from the context it's being used. Could you elaborate or give me an example please.

================
Comment at: include/clang/AST/OperationKinds.def:328
@@ +327,3 @@
+// Convert an integer initializer to an OpenCL sampler initializer.
+CAST_OPERATION(IntToOCLSamplerInitializer)
+
----------------
yaxunl wrote:
> 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.
I don't understand why this is needed? The initialization function only replaces the initializer of a sampler type. So this call only has to be emitted on the conversion int->sampler.

================
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">;
----------------
yaxunl wrote:
> 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.
Could we avoid having SPIR incompatible code instead?

================
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\"">;
----------------
yaxunl wrote:
> Anastasia wrote:
> > Any reason to have this flag and support different sampler representations in Clang?
> Give user some time for transition.
It doesn't look such a big and risky change to add special flag and maintain two different representations. I would rather go for cleanup of old code here.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:52
@@ +51,3 @@
+      return llvm::PointerType::get(llvm::StructType::create(
+                           Ctx, "__sampler"),
+                           CGM.getContext().getTargetAddressSpace(
----------------
yaxunl wrote:
> 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.
Would it work if this function uses OpenCL original types and get compiled with Clang too?


http://reviews.llvm.org/D21567