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