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

[Anastasia Stulova via Phabricator via cfe-commits]

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