[PATCH] D60763: Prototype OpenCL BIFs using Tablegen

[Pierre via Phabricator via cfe-commits]

Pierre added inline comments.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:65
+class QualType<string _AccessMethod,string _Name> {
+  // How to get the QualType. Can be one of ("field", "func")
+  string AccessMethod = _AccessMethod;
----------------
Anastasia wrote:
> I don't think "field" and "func" are clear at this point.
This field wasn't necessary and has been removed


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:211-212
+  def float_t   : Type<"float", QualType<"field", "FloatTy">>;
+  def double_t  : Type<"double", QualType<"field", "DoubleTy">>;
+  def half_t    : Type<"half", QualType<"field", "HalfTy">>;
+}
----------------
Anastasia wrote:
> Nicola wrote:
> > Can half and double types and builtins be made dependent on extensions or configurable? The half datatype needs the cl_khr_fp16 extension, while double needs CL_DEVICE_DOUBLE_FP_CONFIG or cl_khr_fp64
> half and double types are already activated by an extension in Clang. This behavior isn't modified here.
> 
> As for builtins there is `Extension` field in Builtin, but I think it's not used in conversion functions at the moment. I guess we should update that.
An "Extension" field will be added in the next patch, so it will be possible to retrieve an extension of a prototype from the types the prototype is using.
E.g.: In the definition of the prototype half cos(half), the prototype will have the extension "cl_khr_fp16" because it uses the "half" type.

This scheme is present for the "half" and "double" types, but also for the "image2d_depth_t" types (extension "image2d_depth_t") and others.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:306
+                "atan", "atanh", "atanpi"] in {
+  foreach type = [float_t, double_t, half_t] in {  // TODO halfx is not part of the OpenCL1.2 spec
+    defm : Bif1<name, [type, type], [1, 1]>;
----------------
I removed the TODO and let the functions with the half prototypes. The fact that prototypes using the "half" types are part of the "cl_khr_fp16" extension (same for the "double" type, and other types) .


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:326
+// example 'foo', to show using 'version'
+def : Builtin<"foo_version", [int_t, PointerType<int_t, global_as>]>;
+let Version = CL20 in {
----------------
Anastasia wrote:
> I think we now need to use some real function here
I changed it to a real function, but this should be changed anyway in the next patch


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:331
+
+// Example showing 'Extension'
+let Extension = "cl_khr_subgroups" in {
----------------
Anastasia wrote:
> I think this is no longer an example so we can change this something like Built-in Subroups Extension...
> 
> Also this should probably moved to the bottom.
I would like to let it as an example for now because there is only one function using the Extension field


================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:30
+kernel void test5(write_only image2d_t img, int2 coord, float4 colour) {
+  write_imagef(img, coord, colour);
+}
----------------
Anastasia wrote:
> I think different overloads can be available in different CL version. @svenvh can we already handle this?
I wrote a TODO to check this later


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:58
+  //        <<double, double>, 35>.
+  std::vector<std::pair<std::vector<Record *>, unsigned>> SignatureSet;
+
----------------
Anastasia wrote:
> I think it's worth checking whether inner vector is a good candidate for using SmallVector of llvm. We can probably just use it with size 3 by default since size is normally between 1-3. Although we could as well add a TODO and address it later if we encounter performance problems.
I added a TODO


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:61
+  // Map the name of a builtin function to its signatures.
+  // Each signature is registered as a pair of:
+  // <pointer to a list of types (a signature),
----------------
Anastasia wrote:
> Why not to just refer to the data structure above or we can alternatively just typedef the vector?
I explained a bit more the purpose of the two fields in the comments.
**SignatureSet **is storing a list of <pointer to the signature, **Index**>. Tablegen is storing lists of types as objects that we can reference. 
**OverloadInfo **is storing, for each function having the same name, a list of  the following pair:
<A pointer to the TableGen "Builtin" instance,
the "**Index**" of its signature in the SignatureSet>

Thus, this "**Index**" value allows functions with different names to have the same signature. By signature I mean the list of types is uses (for float cos(float), the signature would be [float, float])

I am not sure I answered the question, but I don't think it is possible to merge the SignatureSet and the OverloadInfo  structures


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:182
+
+  std::vector<std::vector<Record *>> Signature;
+
----------------
Anastasia wrote:
> Is this even used?
Actually no


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60763/new/

https://reviews.llvm.org/D60763