[PATCH] D65456: [OpenCL] Add generic type handling for builtin functions

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 1 07:38:28 PDT 2019

Anastasia added inline comments.

Comment at: clang/lib/Sema/OpenCLBuiltins.td:51
-// Helper class to store type access qualifiers (volatile, const, ...).
-class Qualifier<string _QualName> {
-  string QualName = _QualName;
Are the qualifiers added elsewhere?

Comment at: clang/lib/Sema/OpenCLBuiltins.td:221
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;
Is this float GenType?

Comment at: clang/lib/Sema/OpenCLBuiltins.td:222
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;
+// Generate basic GenTypes.  Names are like: GenTypeFloatVecAndScalar.
Would it make sense to use the same name scheme as below?

Comment at: clang/lib/Sema/SemaLookup.cpp:680
+/// \param OpenCLBuiltin (in) The signature currently handled.
+/// \param GenTypeMaxCnt (out) Maximum number of types contained in a generic
+///        type used as return type or as argument.
What if both return and args use GenType, does `GenTypeMaxCnt` return max of all? Or do they have to align?

Comment at: clang/lib/Sema/SemaLookup.cpp:708
+  }
+  for (unsigned Index = 0; Index < ArgTypes.size(); Index++) {
+    unsigned TypeCntAtIndex = ArgTypes[Index].size();
I don't get this logic?

Comment at: clang/lib/Sema/SemaLookup.cpp:755
-/// \param S The Sema instance
-/// \param LR  The LookupResult instance
-/// \param II  The identifier being resolved
-/// \param Index  The list of prototypes starts at Index in OpenCLBuiltins[]
-/// \param Len  The list of prototypes has Len elements
-static void InsertOCLBuiltinDeclarations(Sema &S, LookupResult &LR,
-                                         IdentifierInfo *II, unsigned Index,
-                                         unsigned Len) {
-  for (unsigned i = 0; i < Len; ++i) {
-    const OpenCLBuiltinDecl &Decl = OpenCLBuiltins[Index - 1 + i];
+/// \param S The Sema instance.
+/// \param LR The LookupResult instance.
Btw you are not marking param as (in) or (out) here.

Comment at: clang/lib/Sema/SemaLookup.cpp:817
-      New->setParams(Params);
+      NewOpenCLBuiltin->addAttr(OverloadableAttr::CreateImplicit(Context));
+      LR.addDecl(NewOpenCLBuiltin);
I guess this should be done conditionally for C++ mode. But perhaps we don't have to do this now. It might be worth adding a FIXME.

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:70
 namespace {
 class BuiltinNameEmitter {
Perhaps not something for this patch, but I am wondering if a better name for this class would be `OpenCLBuiltinTrieEmitter`?

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:94
+  // \param List (out) List to fill with the extracted types.
+  void ExtractEnumTypes(std::vector<Record *> &Types,
+                        StringMap<bool> &TypesSeen, std::string &Output,
I am a bit confused about the purpose of this function as input and output seem to be both vectors of the Record. It might make sense to explain the difference. :)

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:225
+  // Generic types are defined at the end of the enum.
+  std::vector<Record *> GenTypes =
I find this comment confusing... may be because it belongs to the code later. I am not sure it adds any value.

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:281
     auto it =
-        std::find_if(SignatureSet.begin(), SignatureSet.end(),
+        std::find_if(SignaturesList.begin(), SignaturesList.end(),
                      [&](const std::pair<std::vector<Record *>, unsigned> &a) {
Is this logic to avoid duplicate signatures? If so it might be worth explaining it.

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:377
+// is returned.
+static void OCL2Qual(ASTContext &Context, const OpenCLTypeStruct &Ty,
+                     std::vector<QualType> &QT) {
I feel it would be good to explain the structure of the function we are trying to generate i.e something like:
- We first generate switch/case statement over all type names that populates the list of type IDs
- Then we walk over the type IDs from the list and map them to the AST type and attributes accordingly.

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:404
+    // the plain scalar types for now; the ExtVector type will be added after
+    // the switch statement such that it is only added for the case matching
+    // the input to the OCL2Qual function.
I am not sure what you are trying to say about ExtVector...

Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:457
+  // Add ExtVector types if this was a generic type, as the switch statement
+  // above only populated the list with scalar types.  This completes the
+  // construction of the Cartesian product of (vector sizes) x (types).
I am really confused as the above comment on line 432 makes examples of non scalar types too i.e. `int4`.

  rC Clang



More information about the cfe-commits mailing list