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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh marked 6 inline comments as done.
svenvh added inline comments.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:116
+// combination of Types and vector sizes.
+//
+// E.g.: If TypeListField = <int, float> and VectorList = <1, 2, 4>, then
----------------
Pierre wrote:
> Maybe it would be worth adding more information on how to use GenTypes. I was thinking of something like this : 
> 
> When using multiple generic types :
>   * The maximal cardinal of the used  GenTypes must be the PGCM of all the cardinals.
>   * The generic types combine as if it was an array like GenType[Type][VecSize].
> I.e: With GT1 = [half, <1, 2>] and GT2 = [float, int, <1, 2>], they will combine as
> <half, float>, <half2, float2>, <half, int>, <half, int2>
> The maximal cardinal of GT1 and GT2 is 4 (= 2 types * 2 vector sizes).
> 4 is the PGCM of GT1 (=2) and GT2 (=4).
I've added some rules about combining GenTypes in the comment below based on your example.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:708
+  }
+  for (unsigned Index = 0; Index < ArgTypes.size(); Index++) {
+    unsigned TypeCntAtIndex = ArgTypes[Index].size();
----------------
svenvh wrote:
> Anastasia wrote:
> > I don't get this logic?
> While trying to clarify this, I realized this checking should probably be moved to the TableGen emitter, as this is checking validity of the .td input so ideally we should check that at compiler compile time.  @Pierre mentioned that this might not be trivial, but I'll have a look at it.
The checking has been moved into `ClangOpenCLBuiltinEmitter.cpp` now, see `VerifySignature`.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:817
       }
-      New->setParams(Params);
+      NewOpenCLBuiltin->addAttr(OverloadableAttr::CreateImplicit(Context));
+      LR.addDecl(NewOpenCLBuiltin);
----------------
Anastasia wrote:
> I guess this should be done conditionally for C++ mode. But perhaps we don't have to do this now. It might be worth adding a FIXME.
Done.


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

https://reviews.llvm.org/D65456