[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)
+
----------------
I can't get why is this necessary to represent initializer as a special Clang type? Could we avoid this additional complexity?

================
Comment at: include/clang/AST/OperationKinds.def:328
@@ +327,3 @@
+// Convert an integer initializer to an OpenCL sampler initializer.
+CAST_OPERATION(IntToOCLSamplerInitializer)
+
----------------
Could we just have only int->sampler conversion? Without having an extra type for initializer?

================
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">;
----------------
I don't understand the description really? Why not compatible?

================
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\"">;
----------------
Any reason to have this flag and support different sampler representations in Clang?

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


http://reviews.llvm.org/D21567