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

[Liu, Yaxun (Sam) via cfe-commits]

+ Brian

Hi Anastasia,

The advantage for translating sampler variable to a global variable with __sampler_initializer type is that it is consistent with OpenCL C++ and SPIRV, so it is easier to translate the IR to SPIRV.

About the type name of sampler_t in LLVM, using a name without `.` allows library functions to use the concrete sampler types directly without casting. Casting not only affects the readability of the library code, but also causes extra efforts in optimizing these codes.
 
Sam

-----Original Message-----
From: Anastasia Stulova [mailto:anastasia.stulova at arm.com] 
Sent: Thursday, July 7, 2016 11:29 AM
To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; anastasia.stulova at arm.com; alexey.bader at intel.com; xiulipan at outlook.com
Cc: Stellard, Thomas <Tom.Stellard at amd.com>; cfe-commits at lists.llvm.org
Subject: Re: [PATCH] D21567: [OpenCL] Generate struct type for sampler_t and function call for the initializer

Anastasia added inline comments.

================
Comment at: include/clang/AST/BuiltinTypes.def:164
@@ +163,3 @@
+// Internal OpenCL sampler initializer type.
+BUILTIN_TYPE(OCLSamplerInit, OCLSamplerInitTy)
+
----------------
> However, we want it to be translated to __sampler_initializer* type.

This is precisely what I am not able to understand. Why do we want it to be  __sampler_initializer* type? I don't think we want that. If it was a sampler type originally in OpenCL, it should be kept a sampler type in LLVM too.

================
Comment at: include/clang/AST/OperationKinds.def:328
@@ +327,3 @@
+// Convert an integer initializer to an OpenCL sampler initializer.
+CAST_OPERATION(IntToOCLSamplerInitializer)
+
----------------
> In codegen, we want to insert a call of __transform_sampler_initializer for the reference of variable a, not the definition of variable a.
 I think the original discussion was to insert the call on the declaration of sampler variable as an initializer. This would allow us to use variable itself "as is" everywhere else.

So if let's say we compile:
  constant sampler_t a = 0;
  kernel void f() {
     g(a);
  }

the code we are generating would be equivalent to:

  kernel void f() {
    __sampler* a = __transform_sampler_initializer(0);
    g(a);
  }

Why are we not using this approach?

I think we discussed later that we could generate a struct of initializers instead of just int (0 -> struct sampler_initializer). Here is the description: http://lists.llvm.org/pipermail/cfe-dev/2016-June/049389.html
But general principle still was as described above...

================
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 see. This is just because OpenCL doesn't allocate actual constants for different allowed sampler modes whereas SPIR does.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:52
@@ +51,3 @@
+                           Ctx, "__sampler"),
+                           CGM.getContext().getTargetAddressSpace(
+                           LangAS::opencl_constant));
----------------
If you do conversion struct ptr <-> opaque ptr in this function, it should be fine? I would imagine images and other OpenCL types have the same requirement.


http://reviews.llvm.org/D21567