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