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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 13 09:17:51 PDT 2019

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



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 {
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. :)



More information about the cfe-commits mailing list