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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
