Anastasia added inline comments.
================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:222
+// GenType definitions.
+def FGenTypeN : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;
+// Generate basic GenTypes. Names are like: GenTypeFloatVecAndScalar.
----------------
svenvh wrote:
> 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.
Ok, it could though be
`GenTypeAllFloats`
`GenTypeF`
Just that naming scheme feels a bit random right now.
================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:124
+//
+// Some rules apply for combining GenericTypes in a declaration:
+// 1. The number of vector sizes must be equal or 1 for all gentypes in a
----------------
What does combining mean?
================
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.
----------------
svenvh wrote:
> 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.
Do we say anywhere they have to be compatible?
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:70
namespace {
class BuiltinNameEmitter {
public:
----------------
svenvh wrote:
> 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)?
Yeah, makes sense!
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:261
+// a signature to contain different GenTypes if these GenTypes represent
+// the same number of actual types.
+//
----------------
I feel what you are checking is whether the vector sizes and types match but
not just that the number of types match...
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:265
+static void VerifySignature(const std::vector<Record *> &Signature,
+ const Record *B) {
+ unsigned GenTypeVecSizes = 1;
----------------
Can we rename B to something meaningful or add a comment explaining it?
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:269
+
+ for (const auto *T : Signature) {
+ if (T->isSubClassOf("GenericType")) {
----------------
I feel there is some black magic here. :)
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
