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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
