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 {
public:
----------------
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`.
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
