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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh 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.
----------------
Anastasia wrote:
> 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.
The convention is a bit subtle perhaps, but it is not random.  For manual definitions that have multiple base types, the convention is typeinfo#GenType#vecinfo.  For generated definitions that have a single base type, the convention is GenType#typeinfo#vecinfo.  Having different conventions prevents the manual definitions from colliding with the generated ones.


================
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
----------------
Anastasia wrote:
> What does combining mean?
Clarified.


================
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:
> 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?
Yes, in OpenCLBuiltins.td near the definition of `GenericType`.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:261
+// a signature to contain different GenTypes if these GenTypes represent
+// the same number of actual types.
+//
----------------
Anastasia wrote:
> I feel what you are checking is whether the vector sizes and types match but not just that the number of types match...
"actual types" here means "final" non-generic types, such as half2, int, short4, etc.

Rephrased as "actual scalar or vector types", does that help?


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:265
+static void VerifySignature(const std::vector<Record *> &Signature,
+                            const Record *B) {
+  unsigned GenTypeVecSizes = 1;
----------------
Anastasia wrote:
> Can we rename B to something meaningful or add a comment explaining it?
Renamed to `BuiltinRec`.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:269
+
+  for (const auto *T : Signature) {
+    if (T->isSubClassOf("GenericType")) {
----------------
Anastasia wrote:
> I feel there is some black magic here. :)
Added some comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65456/new/

https://reviews.llvm.org/D65456