[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:
> 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.
For example, 

  constant sampler_t a = 0;
  kernel void f(sampler_t b) {
     g(a);
     g(b);
   }

Let's say we don't introduce a `OCLSamplerInitTy` in AST.  We only introduce `OCLSamplerTy` to represent `sampler_t` in AST. Then both variables `a` and `b` have `OCLSamplerTy` type in AST.

In codegen, type translation is done by `CodeGenTypes::ConvertType`, which only accepts a `QualType` argument, which means we can only translate an AST type to an LLVM type based on the AST type itself. Therefore we can only translate `OCLSamplerTy` to one LLVM type. Let's say we translate `OCLSamplerTy` to `__sampler*` type. Then variable `a` will be translated to `__sampler*` type. However, we want it to be translated to `__sampler_initializer*` type. To do that, we have to customize `CodeGenModule::EmitGlobalVarDefinition` to translate global variables with `OCLSamplerTy` type specially, which will be ugly.

Basically we cannot avoid the complexity of translating the type for variable `a`. The complexity is only moved from AST to Codegen. If we introduce `OCLSamplerInitTy` in AST, we don't need to translate the type of variable `a` specially. It seems to be a more elegant solution to me.



================
Comment at: include/clang/AST/OperationKinds.def:328
@@ +327,3 @@
+// Convert an integer initializer to an OpenCL sampler initializer.
+CAST_OPERATION(IntToOCLSamplerInitializer)
+
----------------
Anastasia wrote:
> 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.
Let's consider this example:

  constant sampler_t a = 0;
  kernel void f() {
     g(a);
   }

In codegen, we want to insert a call of `__transform_sampler_initializer` for the reference of variable `a`, not the definition of variable `a`. If we introduce a cast `SamplerInitializerToSamplerCast`, we can represent that in AST equivalent to:

  __sampler_initializer *a = IntToSamplerInitializerCast(0);
  kernel void f() {
     g(SamplerInitializerToSamplerCast(a));
  }

We can do a straight forward translation by translating `SamplerInitializerToSamplerCast` to function call of `__transform_sampler_initializer`.

If we do not have `SamplerInitializerToSamplerCast`, the AST will be equivalent to:

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

Can we translate `IntToSamplerCast` to `__transform_sampler_initializer`? No. Because translation of `IntToSamplerCast` is done at translating global variable `a`, which happens before translating instructions belonging to a function, whereas we need to insert a function call of `__transform_sampler_initializer` when translating function call of `g`. Since we do not have a landmark for inserting function call of `__transform_sampler_initializer`, we have to customize `CodeGenFunction::EmitCall` to handle argument being a global variable of type `__sampler` specially, which will be ugly.

However we do have a simple solution for this if we do a post-processing of the generated LLVM IR in `CodeGenModule::Release`. Basically we iterate all uses of global variable `a` and replace them with a function call of `__transform_sampler_initializer`. By doing this we can use a simple AST without introducing the sampler initializer type and the cast from sampler initializer to sampler. Since `CodeGenModule::Release` already contains quite a few post-prcessing's for LLVM IR, I think it is reasonable to do some OpenCL specific post-processing there. We can introduce a new member CGOpenCLRuntime::postprocessIR for this purpose.


================
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:
> 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?
It is the user's OpenCL source code which may contain SPIR incompatible integer literal value for sampler, e.g. a user may write

  sampler_t s = 0;

This will result in a sampler value not compatible with SPIR. We cannot prevent user from doing that, but we can emit a warning for that.

================
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:
> 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.
I can remove the support of old sampler representation.

================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:52
@@ +51,3 @@
+      return llvm::PointerType::get(llvm::StructType::create(
+                           Ctx, "__sampler"),
+                           CGM.getContext().getTargetAddressSpace(
----------------
Anastasia wrote:
> 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?
Probably not. The OpenCL type `sampler_t` is translated to a pointer to an opaque struct. However we need to return a pointer to a concrete struct type since we need to fill the struct in `__transform_sampler_initializer`.


http://reviews.llvm.org/D21567