[PATCH] D60763: Prototype OpenCL BIFs using Tablegen

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:243
+// ========== Builtin definitions ==========
+// This section defines builtin functions found in the OpenCL 1.2 specification
+
----------------
Anastasia wrote:
> found in -> from
ping!


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:13
+// prototypes. In case of an unresolved function names in OpenCL, Clang will
+// check the function is not a builtin function.
+//
----------------
How about - Clang will check for functions described in this file?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:26
+//                            Base definitions
+// These classes/ definitions are representing simple information.
+//===----------------------------------------------------------------------===//
----------------
simple -> basic


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:305
+
+// ========== Builtin definitions (Examples) ==========
+
----------------
Why Examples?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:307
+
+// Example to show use of the "Version" field.
+let Version = CL20 in {
----------------
Let's remove Example please!


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:312
+
+// Example to show use of the "Extension" field.
+let Extension = "cl_khr_subgroups" in {
----------------
Please rename Example here too! We can just say something like builting functions that belong to extensions.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:679
+// prototypes are added to the LookUpResult.
+// S      : The sema instance
+// LR     : The lookupResult instance
----------------
Anastasia wrote:
> This does not match Clang documentation format. Can you please align with other functions in this file.
Ping. The format here still doesn't match the rest. Here is an example how we document functions.




```
 6205 /// Look up the special member function that would be called by a special
 6206 /// member function for a subobject of class type.
 6207 ///
 6208 /// \param Class The class type of the subobject.
 6209 /// \param CSM The kind of special member function.
 6210 /// \param FieldQuals If the subobject is a field, its cv-qualifiers.
 6211 /// \param ConstRHS True if this is a copy operation with a const object
 6212 ///        on its RHS, that is, if the argument to the outer special member
 6213 ///        function is 'const' and this is not a field marked 'mutable'.
 6214 static Sema::SpecialMemberOverloadResult lookupCallFromSpecialMember(
 6215     Sema &S, CXXRecordDecl *Class, Sema::CXXSpecialMember CSM,
 6216     unsigned FieldQuals, bool ConstRHS) {

```



================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:15
+
+kernel void version(float a) {
+  size_t b;
----------------
I think we should name the functions based on what BIFs they tests. Otherwise it will become too hard to navigate and track.


================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:28
+
+// TODO Maybe overloads can be available in different CL version. Check this.
+#ifdef CL20
----------------
Is this TODO still needed?


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:97
+  // static QualType OCL2Qual(ASTContext &Context, OpenCLType Ty);
+  // TODO: Should not need clang::
+  void EmitQualTypeFinder();
----------------
Anastasia wrote:
> Should we remove the TODO?
ping!

line 97 now.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:107
+  //        <<double, double>, 35>.
+  // TODO: Check the other types of vector available for performance purpose
+  std::vector<std::pair<std::vector<Record *>, unsigned>> SignatureSet;
----------------
Is this TODO still relevant?


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:126
+void BuiltinNameEmitter::Emit() {
+  // Generate the file containing OpenCL builtin functions checking code.
+
----------------
This should be moved above?


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

https://reviews.llvm.org/D60763