svenvh marked 23 inline comments as done. svenvh added a subscriber: Pierre. svenvh 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; ---------------- Anastasia wrote: > Are the qualifiers added elsewhere? Good point, this change should be part of the next patch (D63442). I've taken it out of this patch. ================ Comment at: clang/lib/Sema/OpenCLBuiltins.td:221 + +// GenType definitions. +def FGenTypeN : GenericType<"FGenTypeN", TLFloat, VecAndScalar>; ---------------- Anastasia wrote: > Is this float GenType? We will be adding more definitions for integers here in followup patches. ================ Comment at: clang/lib/Sema/OpenCLBuiltins.td:222 +// GenType definitions. +def FGenTypeN : GenericType<"FGenTypeN", TLFloat, VecAndScalar>; +// Generate basic GenTypes. Names are like: GenTypeFloatVecAndScalar. ---------------- Anastasia wrote: > Would it make sense to use the same name scheme as below? No, because FGenType is for all floating point types (half / float / double), as it uses the `TLFloat` type list. `GenTypeFloatVecAndScalar` is for the float type (i.e. fp32) only. I will update the comments of both definitions to clarify the difference. ================ 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. ---------------- Anastasia wrote: > What if both return and args use GenType, does `GenTypeMaxCnt` return max of > all? Or do they have to align? They have to be compatible, otherwise we will hit the `llvm_reachable` below. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:708 + } + for (unsigned Index = 0; Index < ArgTypes.size(); Index++) { + unsigned TypeCntAtIndex = ArgTypes[Index].size(); ---------------- Anastasia wrote: > I don't get this logic? While trying to clarify this, I realized this checking should probably be moved to the TableGen emitter, as this is checking validity of the .td input so ideally we should check that at compiler compile time. @Pierre mentioned that this might not be trivial, but I'll have a look at it. ================ Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:70 namespace { class BuiltinNameEmitter { public: ---------------- Anastasia wrote: > Perhaps not something for this patch, but I am wondering if a better name for > this class would be `OpenCLBuiltinTrieEmitter`? It's emitting more than a trie; also tables and structs. How about `OpenCLBuiltinEmitter` (which aligns nicely with the file name)? ================ 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, ---------------- Anastasia wrote: > 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. :) Clarified comment. ================ Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:225 + + // Generic types are defined at the end of the enum. + std::vector<Record *> GenTypes = ---------------- Anastasia wrote: > I find this comment confusing... may be because it belongs to the code later. > I am not sure it adds any value. Elaborated. ================ 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) { ---------------- Anastasia wrote: > Is this logic to avoid duplicate signatures? If so it might be worth > explaining it. Added comment. ================ Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:377 +// is returned. +static void OCL2Qual(ASTContext &Context, const OpenCLTypeStruct &Ty, + std::vector<QualType> &QT) { ---------------- Anastasia wrote: > 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. Added comment. ================ 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. ---------------- Anastasia wrote: > I am not sure what you are trying to say about ExtVector... Rephrased. ================ 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). ---------------- Anastasia wrote: > I am really confused as the above comment on line 432 makes examples of non > scalar types too i.e. `int4`. I guess you got confused about the two different steps; I have rephrased the comment on line 432. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65456/new/ https://reviews.llvm.org/D65456 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits