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

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369253: [OpenCL] Add generic type handling for builtin 
functions (authored by svenvh, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65456?vs=215086=215859#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65456

Files:
  cfe/trunk/lib/Sema/OpenCLBuiltins.td
  cfe/trunk/lib/Sema/SemaLookup.cpp
  cfe/trunk/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  cfe/trunk/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: cfe/trunk/lib/Sema/SemaLookup.cpp
===
--- cfe/trunk/lib/Sema/SemaLookup.cpp
+++ cfe/trunk/lib/Sema/SemaLookup.cpp
@@ -673,76 +673,148 @@
 D->dump();
 }
 
-/// When trying to resolve a function name, if the isOpenCLBuiltin function
-/// defined in "OpenCLBuiltins.inc" returns a non-null , then the
-/// identifier is referencing an OpenCL builtin function. Thus, all its
-/// prototypes are added to the LookUpResult.
-///
-/// \param S The Sema instance
-/// \param LR  The LookupResult instance
-/// \param II  The identifier being resolved
-/// \param Index  The list of prototypes starts at Index in OpenCLBuiltins[]
-/// \param Len  The list of prototypes has Len elements
-static void InsertOCLBuiltinDeclarations(Sema , LookupResult ,
- IdentifierInfo *II, unsigned Index,
- unsigned Len) {
-
-  for (unsigned i = 0; i < Len; ++i) {
-const OpenCLBuiltinDecl  = OpenCLBuiltins[Index - 1 + i];
-ASTContext  = S.Context;
+/// Get the QualType instances of the return type and arguments for an OpenCL
+/// builtin function signature.
+/// \param Context (in) The Context instance.
+/// \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.
+///Only meaningful for generic types, otherwise equals 1.
+/// \param RetTypes (out) List of the possible return types.
+/// \param ArgTypes (out) List of the possible argument types.  For each
+///argument, ArgTypes contains QualTypes for the Cartesian product
+///of (vector sizes) x (types) .
+static void GetQualTypesForOpenCLBuiltin(
+ASTContext , const OpenCLBuiltinStruct ,
+unsigned , std::vector ,
+SmallVector, 5> ) {
+  // Get the QualType instances of the return types.
+  unsigned Sig = SignatureTable[OpenCLBuiltin.SigTableIndex];
+  OCL2Qual(Context, TypeTable[Sig], RetTypes);
+  GenTypeMaxCnt = RetTypes.size();
+
+  // Get the QualType instances of the arguments.
+  // First type is the return type, skip it.
+  for (unsigned Index = 1; Index < OpenCLBuiltin.NumTypes; Index++) {
+std::vector Ty;
+OCL2Qual(Context,
+TypeTable[SignatureTable[OpenCLBuiltin.SigTableIndex + Index]], Ty);
+ArgTypes.push_back(Ty);
+GenTypeMaxCnt = (Ty.size() > GenTypeMaxCnt) ? Ty.size() : GenTypeMaxCnt;
+  }
+}
 
-// Ignore this BIF if the version is incorrect.
-if (Context.getLangOpts().OpenCLVersion < Decl.Version)
-  continue;
+/// Create a list of the candidate function overloads for an OpenCL builtin
+/// function.
+/// \param Context (in) The ASTContext instance.
+/// \param GenTypeMaxCnt (in) Maximum number of types contained in a generic
+///type used as return type or as argument.
+///Only meaningful for generic types, otherwise equals 1.
+/// \param FunctionList (out) List of FunctionTypes.
+/// \param RetTypes (in) List of the possible return types.
+/// \param ArgTypes (in) List of the possible types for the arguments.
+static void
+GetOpenCLBuiltinFctOverloads(ASTContext , unsigned GenTypeMaxCnt,
+ std::vector ,
+ std::vector ,
+ SmallVector, 5> ) {
+  FunctionProtoType::ExtProtoInfo PI;
+  PI.Variadic = false;
+
+  // Create FunctionTypes for each (gen)type.
+  for (unsigned IGenType = 0; IGenType < GenTypeMaxCnt; IGenType++) {
+SmallVector ArgList;
+
+for (unsigned A = 0; A < ArgTypes.size(); A++) {
+  // Builtins such as "max" have an "sgentype" argument that represents
+  // the corresponding scalar type of a gentype.  The number of gentypes
+  // must be a multiple of the number of sgentypes.
+  assert(GenTypeMaxCnt % ArgTypes[A].size() == 0 &&
+ "argument type count not compatible with gentype type count");
+  unsigned Idx = IGenType % ArgTypes[A].size();
+  ArgList.push_back(ArgTypes[A][Idx]);
+}
+
+FunctionList.push_back(Context.getFunctionType(
+RetTypes[(RetTypes.size() != 1) ? IGenType : 0], ArgList, PI));
+  }
+}
 
-FunctionProtoType::ExtProtoInfo PI;
-PI.Variadic = false;
+/// When trying 

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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks!


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

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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 215086.
svenvh marked 15 inline comments as done.
svenvh added a comment.

- Update comments as per review comments.
- Rename iterator `List` to `VecSizes` in OpenCLBuiltins.td
- Format GenericType definition.


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

https://reviews.llvm.org/D65456

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -15,12 +15,39 @@
 //
 // For a successful lookup of e.g. the "cos" builtin, isOpenCLBuiltin("cos")
 // returns a pair .
-// OpenCLBuiltins[Index] to OpenCLBuiltins[Index + Len] contains the pairs
+// BuiltinTable[Index] to BuiltinTable[Index + Len] contains the pairs
 //  of the overloads of "cos".
-// OpenCLSignature[SigIndex] to OpenCLSignature[SigIndex + SigLen] contains
-// one of the signatures of "cos". The OpenCLSignature entry can be
-// referenced by other functions, i.e. "sin", since multiple OpenCL builtins
-// share the same signature.
+// SignatureTable[SigIndex] to SignatureTable[SigIndex + SigLen] contains
+// one of the signatures of "cos". The SignatureTable entry can be
+// referenced by other functions, e.g. "sin", to exploit the fact that
+// many OpenCL builtins share the same signature.
+//
+// The file generated by this TableGen emitter contains the following:
+//
+//  * Structs and enums to represent types and function signatures.
+//
+//  * OpenCLTypeStruct TypeTable[]
+//Type information for return types and arguments.
+//
+//  * unsigned SignatureTable[]
+//A list of types representing function signatures.  Each entry is an index
+//into the above TypeTable.  Multiple entries following each other form a
+//signature, where the first entry is the return type and subsequent
+//entries are the argument types.
+//
+//  * OpenCLBuiltinStruct BuiltinTable[]
+//Each entry represents one overload of an OpenCL builtin function and
+//consists of an index into the SignatureTable and the number of arguments.
+//
+//  * std::pair isOpenCLBuiltin(llvm::StringRef Name)
+//Find out whether a string matches an existing OpenCL builtin function
+//name and return an index into BuiltinTable and the number of overloads.
+//
+//  * void OCL2Qual(ASTContext&, OpenCLTypeStruct, std::vector&)
+//Convert an OpenCLTypeStruct type to a list of QualType instances.
+//One OpenCLTypeStruct can represent multiple types, primarily when using
+//GenTypes.
+//
 //===--===//
 
 #include "llvm/ADT/MapVector.h"
@@ -57,34 +84,47 @@
   // The output file.
   raw_ostream 
 
-  // Emit the enums and structs.
+  // Helper function for BuiltinNameEmitter::EmitDeclarations.  Generate enum
+  // definitions in the Output string parameter, and save their Record instances
+  // in the List parameter.
+  // \param Types (in) List containing the Types to extract.
+  // \param TypesSeen (inout) List containing the Types already extracted.
+  // \param Output (out) String containing the enums to emit in the output file.
+  // \param List (out) List containing the extracted Types, except the Types in
+  //TypesSeen.
+  void ExtractEnumTypes(std::vector ,
+StringMap , std::string ,
+std::vector );
+
+  // Emit the enum or struct used in the generated file.
+  // Populate the TypeList at the same time.
   void EmitDeclarations();
 
-  // Parse the Records generated by TableGen and populate OverloadInfo and
-  // SignatureSet.
+  // Parse the Records generated by TableGen to populate the SignaturesList,
+  // FctOverloadMap and TypeMap.
   void GetOverloads();
 
-  // Emit the OpenCLSignature table. This table contains all possible
-  // signatures, and is a struct OpenCLType. A signature is composed of a
-  // return type (mandatory), followed by zero or more argument types.
+  // Emit the TypeTable containing all types used by OpenCL builtins.
+  void EmitTypeTable();
+
+  // Emit the SignatureTable. This table contains all the possible signatures.
+  // A signature is stored as a list of indexes of the TypeTable.
+  // The first index references the return type (mandatory), and the followings
+  // reference its arguments.
   // E.g.:
-  // // 12
-  // { OCLT_uchar, 4, clang::LangAS::Default, false },
-  // { OCLT_float, 4, clang::LangAS::Default, false },
-  // This means that index 12 represents a signature
-  //   - returning a uchar vector of 4 elements, and
-  //   - taking as first argument a float vector of 4 elements.
+  // 15, 2, 15 can represent a function with the signature:
+  // int func(float, 

[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 ,
+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

[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 ,
+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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 =  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
> , , , 
> 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 214169.
svenvh added a comment.

- Move checking of GenType compatibility from SemaLookup to TableGen emitter.
- Provide more elaborate explanation about combining GenTypes in a declaration.
- Add `max`/`min` definitions to cover "sgentype" behavior and add test for 
`max`.
- Minor scattered comment improvements.

Combining GenTypes in an incorrect way will now produce an error from TableGen, 
e.g.:

  OpenCLBuiltins.td:1601:1: error: number of vector sizes should be equal or 1 
for all gentypes in a declaration
  def : Builtin<"crashme", [GenTypeFloatVecNoScalar, GenTypeFloatVecAndScalar]>
  ^
  
  OpenCLBuiltins.td:1602:1: error: number of types should be equal or 1 for all 
gentypes in a declaration
  def : Builtin<"crashme2", [IGenTypeN, FGenTypeN]>;
  ^


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

https://reviews.llvm.org/D65456

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -15,12 +15,39 @@
 //
 // For a successful lookup of e.g. the "cos" builtin, isOpenCLBuiltin("cos")
 // returns a pair .
-// OpenCLBuiltins[Index] to OpenCLBuiltins[Index + Len] contains the pairs
+// BuiltinTable[Index] to BuiltinTable[Index + Len] contains the pairs
 //  of the overloads of "cos".
-// OpenCLSignature[SigIndex] to OpenCLSignature[SigIndex + SigLen] contains
-// one of the signatures of "cos". The OpenCLSignature entry can be
-// referenced by other functions, i.e. "sin", since multiple OpenCL builtins
-// share the same signature.
+// SignatureTable[SigIndex] to SignatureTable[SigIndex + SigLen] contains
+// one of the signatures of "cos". The SignatureTable entry can be
+// referenced by other functions, e.g. "sin", to exploit the fact that
+// many OpenCL builtins share the same signature.
+//
+// The file generated by this TableGen emitter contains the following:
+//
+//  * Structs and enums to represent types and function signatures.
+//
+//  * OpenCLTypeStruct TypeTable[]
+//Type information for return types and arguments.
+//
+//  * unsigned SignatureTable[]
+//A list of types representing function signatures.  Each entry is an index
+//into the above TypeTable.  Multiple entries following each other form a
+//signature, where the first entry is the return type and subsequent
+//entries are the argument types.
+//
+//  * OpenCLBuiltinStruct BuiltinTable[]
+//Each entry represents one overload of an OpenCL builtin function and
+//consists of an index into the SignatureTable and the number of arguments.
+//
+//  * std::pair isOpenCLBuiltin(llvm::StringRef Name)
+//Find out whether a string matches an existing OpenCL builtin function
+//name and return an index into BuiltinTable and the number of overloads.
+//
+//  * void OCL2Qual(ASTContext&, OpenCLTypeStruct, std::vector&)
+//Convert an OpenCLTypeStruct type to a list of QualType instances.
+//One OpenCLTypeStruct can represent multiple types, primarily when using
+//GenTypes.
+//
 //===--===//
 
 #include "llvm/ADT/MapVector.h"
@@ -57,34 +84,47 @@
   // The output file.
   raw_ostream 
 
-  // Emit the enums and structs.
+  // Helper function for BuiltinNameEmitter::EmitDeclarations.  Generate enum
+  // definitions in the Output string parameter, and save their Record instances
+  // in the List parameter.
+  // \param Types (in) List containing the Types to extract.
+  // \param TypesSeen (inout) List containing the Types already extracted.
+  // \param Output (out) String containing the enums to emit in the output file.
+  // \param List (out) List containing the extracted Types, except the Types in
+  //TypesSeen.
+  void ExtractEnumTypes(std::vector ,
+StringMap , std::string ,
+std::vector );
+
+  // Emit the enum or struct used in the generated file.
+  // Populate the TypeList at the same time.
   void EmitDeclarations();
 
-  // Parse the Records generated by TableGen and populate OverloadInfo and
-  // SignatureSet.
+  // Parse the Records generated by TableGen to populate the SignaturesList,
+  // FctOverloadMap and TypeMap.
   void GetOverloads();
 
-  // Emit the OpenCLSignature table. This table contains all possible
-  // signatures, and is a struct OpenCLType. A signature is composed of a
-  // return type (mandatory), followed by zero or more argument types.
+  // Emit the TypeTable containing all types used by OpenCL builtins.
+  void EmitTypeTable();
+
+  // Emit the SignatureTable. This table contains all the possible signatures.
+  // A 

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

[Pierre GONDOIS via Phabricator via cfe-commits]

Pierre added a comment.

Just one comment,
Thank you again for making everything clearer :)




Comment at: clang/lib/Sema/OpenCLBuiltins.td:116
+// combination of Types and vector sizes.
+//
+// E.g.: If TypeListField =  and VectorList = <1, 2, 4>, then

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
, , , 
The maximal cardinal of GT1 and GT2 is 4 (= 2 types * 2 vector sizes).
4 is the PGCM of GT1 (=2) and GT2 (=4).


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

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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 213904.
svenvh marked 9 inline comments as done.
svenvh added a comment.

Addressing review comments.


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

https://reviews.llvm.org/D65456

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -15,12 +15,39 @@
 //
 // For a successful lookup of e.g. the "cos" builtin, isOpenCLBuiltin("cos")
 // returns a pair .
-// OpenCLBuiltins[Index] to OpenCLBuiltins[Index + Len] contains the pairs
+// BuiltinTable[Index] to BuiltinTable[Index + Len] contains the pairs
 //  of the overloads of "cos".
-// OpenCLSignature[SigIndex] to OpenCLSignature[SigIndex + SigLen] contains
-// one of the signatures of "cos". The OpenCLSignature entry can be
-// referenced by other functions, i.e. "sin", since multiple OpenCL builtins
-// share the same signature.
+// SignatureTable[SigIndex] to SignatureTable[SigIndex + SigLen] contains
+// one of the signatures of "cos". The SignatureTable entry can be
+// referenced by other functions, e.g. "sin", to exploit the fact that
+// many OpenCL builtins share the same signature.
+//
+// The file generated by this TableGen emitter contains the following:
+//
+//  * Structs and enums to represent types and function signatures.
+//
+//  * OpenCLTypeStruct TypeTable[]
+//Type information for return types and arguments.
+//
+//  * unsigned SignatureTable[]
+//A list of types representing function signatures.  Each entry is an index
+//into the above TypeTable.  Multiple entries following each other form a
+//signature, where the first entry is the return type and subsequent
+//entries are the argument types.
+//
+//  * OpenCLBuiltinStruct BuiltinTable[]
+//Each entry represents one overload of an OpenCL builtin function and
+//consists of an index into the SignatureTable and the number of arguments.
+//
+//  * std::pair isOpenCLBuiltin(llvm::StringRef Name)
+//Find out whether a string matches an existing OpenCL builtin function
+//name and return an index into BuiltinTable and the number of overloads.
+//
+//  * void OCL2Qual(ASTContext&, OpenCLTypeStruct, std::vector&)
+//Convert an OpenCLTypeStruct type to a list of QualType instances.
+//One OpenCLTypeStruct can represent multiple types, primarily when using
+//GenTypes.
+//
 //===--===//
 
 #include "llvm/ADT/MapVector.h"
@@ -57,34 +84,47 @@
   // The output file.
   raw_ostream 
 
-  // Emit the enums and structs.
+  // Helper function for BuiltinNameEmitter::EmitDeclarations.  Generate enum
+  // definitions in the Output string parameter, and save their Record instances
+  // in the List parameter.
+  // \param Types (in) List containing the Types to extract.
+  // \param TypesSeen (inout) List containing the Types already extracted.
+  // \param Output (out) String containing the enums to emit in the output file.
+  // \param List (out) List containing the extracted Types, except the Types in
+  //TypesSeen.
+  void ExtractEnumTypes(std::vector ,
+StringMap , std::string ,
+std::vector );
+
+  // Emit the enum or struct used in the generated file.
+  // Populate the TypeList at the same time.
   void EmitDeclarations();
 
-  // Parse the Records generated by TableGen and populate OverloadInfo and
-  // SignatureSet.
+  // Parse the Records generated by TableGen to populate the SignaturesList,
+  // FctOverloadMap and TypeMap.
   void GetOverloads();
 
-  // Emit the OpenCLSignature table. This table contains all possible
-  // signatures, and is a struct OpenCLType. A signature is composed of a
-  // return type (mandatory), followed by zero or more argument types.
+  // Emit the TypeTable containing all types used by OpenCL builtins.
+  void EmitTypeTable();
+
+  // Emit the SignatureTable. This table contains all the possible signatures.
+  // A signature is stored as a list of indexes of the TypeTable.
+  // The first index references the return type (mandatory), and the followings
+  // reference its arguments.
   // E.g.:
-  // // 12
-  // { OCLT_uchar, 4, clang::LangAS::Default, false },
-  // { OCLT_float, 4, clang::LangAS::Default, false },
-  // This means that index 12 represents a signature
-  //   - returning a uchar vector of 4 elements, and
-  //   - taking as first argument a float vector of 4 elements.
+  // 15, 2, 15 can represent a function with the signature:
+  // int func(float, int)
+  // The "int" type being at the index 15 in the TypeTable.
   void EmitSignatureTable();
 
-  // Emit 

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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh marked 23 inline comments as done.
svenvh added a subscriber: Pierre.
svenvh added inline comments.



Comment at: clang/lib/Sema/OpenCLBuiltins.td:51
-// Helper class to store type access qualifiers (volatile, const, ...).
-class Qualifier {
-  string QualName = _QualName;

Anastasia wrote:
> Are the qualifiers added elsewhere?
Good point, this change should be part of the next patch (D63442).  I've taken 
it out of this patch.



Comment at: clang/lib/Sema/OpenCLBuiltins.td:221
+
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;

Anastasia wrote:
> Is this float GenType?
We will be adding more definitions for integers here in followup patches.



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



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



Comment at: clang/lib/Sema/SemaLookup.cpp:708
+  }
+  for (unsigned Index = 0; Index < ArgTypes.size(); Index++) {
+unsigned TypeCntAtIndex = ArgTypes[Index].size();

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.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:70
 namespace {
 class BuiltinNameEmitter {
 public:

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)?



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:94
+  // \param List (out) List to fill with the extracted types.
+  void ExtractEnumTypes(std::vector ,
+StringMap , std::string ,

Anastasia wrote:
> I am a bit confused about the purpose of this function as input and output 
> seem to be both vectors of the Record. It might make sense to explain the 
> difference. :)
Clarified comment.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:225
+
+  // Generic types are defined at the end of the enum.
+  std::vector GenTypes =

Anastasia wrote:
> I find this comment confusing... may be because it belongs to the code later. 
> I am not sure it adds any value.
Elaborated.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:281
 auto it =
-std::find_if(SignatureSet.begin(), SignatureSet.end(),
+std::find_if(SignaturesList.begin(), SignaturesList.end(),
  [&](const std::pair, unsigned> ) {

Anastasia wrote:
> Is this logic to avoid duplicate signatures? If so it might be worth 
> explaining it.
Added comment.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:377
+// is returned.
+static void OCL2Qual(ASTContext , const OpenCLTypeStruct ,
+ std::vector ) {

Anastasia wrote:
> I feel it would be good to explain the structure of the function we are 
> trying to generate i.e something like:
> - We first generate switch/case statement over all type names that populates 
> the list of type IDs
> - Then we walk over the type IDs from the list and map them to the AST type 
> and attributes accordingly.
Added comment.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:404
+// the plain scalar types for now; the ExtVector type will be added after
+// the switch statement such that it is only added for the case matching
+// the input to the OCL2Qual function.

Anastasia wrote:
> I am not sure what you are trying to say about ExtVector...

[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:51
-// Helper class to store type access qualifiers (volatile, const, ...).
-class Qualifier {
-  string QualName = _QualName;

Are the qualifiers added elsewhere?



Comment at: clang/lib/Sema/OpenCLBuiltins.td:221
+
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;

Is this float GenType?



Comment at: clang/lib/Sema/OpenCLBuiltins.td:222
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;
+// Generate basic GenTypes.  Names are like: GenTypeFloatVecAndScalar.

Would it make sense to use the same name scheme as below?



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.

What if both return and args use GenType, does `GenTypeMaxCnt` return max of 
all? Or do they have to align?



Comment at: clang/lib/Sema/SemaLookup.cpp:708
+  }
+  for (unsigned Index = 0; Index < ArgTypes.size(); Index++) {
+unsigned TypeCntAtIndex = ArgTypes[Index].size();

I don't get this logic?



Comment at: clang/lib/Sema/SemaLookup.cpp:755
 ///
-/// \param S The Sema instance
-/// \param LR  The LookupResult instance
-/// \param II  The identifier being resolved
-/// \param Index  The list of prototypes starts at Index in OpenCLBuiltins[]
-/// \param Len  The list of prototypes has Len elements
-static void InsertOCLBuiltinDeclarations(Sema , LookupResult ,
- IdentifierInfo *II, unsigned Index,
- unsigned Len) {
-
-  for (unsigned i = 0; i < Len; ++i) {
-const OpenCLBuiltinDecl  = OpenCLBuiltins[Index - 1 + i];
+/// \param S The Sema instance.
+/// \param LR The LookupResult instance.

Btw you are not marking param as (in) or (out) here.



Comment at: clang/lib/Sema/SemaLookup.cpp:817
   }
-  New->setParams(Params);
+  NewOpenCLBuiltin->addAttr(OverloadableAttr::CreateImplicit(Context));
+  LR.addDecl(NewOpenCLBuiltin);

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.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:70
 namespace {
 class BuiltinNameEmitter {
 public:

Perhaps not something for this patch, but I am wondering if a better name for 
this class would be `OpenCLBuiltinTrieEmitter`?



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:94
+  // \param List (out) List to fill with the extracted types.
+  void ExtractEnumTypes(std::vector ,
+StringMap , std::string ,

I am a bit confused about the purpose of this function as input and output seem 
to be both vectors of the Record. It might make sense to explain the 
difference. :)



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:225
+
+  // Generic types are defined at the end of the enum.
+  std::vector GenTypes =

I find this comment confusing... may be because it belongs to the code later. I 
am not sure it adds any value.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:281
 auto it =
-std::find_if(SignatureSet.begin(), SignatureSet.end(),
+std::find_if(SignaturesList.begin(), SignaturesList.end(),
  [&](const std::pair, unsigned> ) {

Is this logic to avoid duplicate signatures? If so it might be worth explaining 
it.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:377
+// is returned.
+static void OCL2Qual(ASTContext , const OpenCLTypeStruct ,
+ std::vector ) {

I feel it would be good to explain the structure of the function we are trying 
to generate i.e something like:
- We first generate switch/case statement over all type names that populates 
the list of type IDs
- Then we walk over the type IDs from the list and map them to the AST type and 
attributes accordingly.



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:404
+// the plain scalar types for now; the ExtVector type will be added after
+// the switch statement such that it is only added for the case matching
+// the input to the OCL2Qual function.

I am not sure what you are trying to say about ExtVector...



Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:457
+  // Add ExtVector types if this was a generic type, as the switch 

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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

Main changes since D63434 :

- Rename List* to Vec*.
- Rename TLnn -> TLAll, TLInt, TLFloat.
- Apply clang-format.
- Improve/update documentation.
- Factor out renaming of base types into separate commit.
- Change return type of OCL2Qual.
- Remove default case from OCL2Qual switch statement: it should be fully 
covering the enum.


Repository:
  rC Clang

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

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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh created this revision.
svenvh added a reviewer: Anastasia.
Herald added subscribers: cfe-commits, kristina, yaxunl.
Herald added a reviewer: rengolin.
Herald added a project: clang.

Generic types are an abstraction of type sets.  It mimics the way
functions are defined in the OpenCL specification.  For example,
floatN can abstract all the vector sizes of the float type.

This allows to

- stick more closely to the specification, which uses generic types;
- factorize definitions of functions with numerous prototypes in the tablegen 
file; and
- reduce the memory impact of functions with many overloads.

Patch by Pierre Gondois and Sven van Haastregt.

Continuation of https://reviews.llvm.org/D63434


Repository:
  rC Clang

https://reviews.llvm.org/D65456

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -15,12 +15,39 @@
 //
 // For a successful lookup of e.g. the "cos" builtin, isOpenCLBuiltin("cos")
 // returns a pair .
-// OpenCLBuiltins[Index] to OpenCLBuiltins[Index + Len] contains the pairs
+// BuiltinTable[Index] to BuiltinTable[Index + Len] contains the pairs
 //  of the overloads of "cos".
-// OpenCLSignature[SigIndex] to OpenCLSignature[SigIndex + SigLen] contains
-// one of the signatures of "cos". The OpenCLSignature entry can be
-// referenced by other functions, i.e. "sin", since multiple OpenCL builtins
-// share the same signature.
+// SignatureTable[SigIndex] to SignatureTable[SigIndex + SigLen] contains
+// one of the signatures of "cos". The SignatureTable entry can be
+// referenced by other functions, e.g. "sin", to exploit the fact that
+// many OpenCL builtins share the same signature.
+//
+// The file generated by this TableGen emitter contains the following:
+//
+//  * Structs and enums to represent types and function signatures.
+//
+//  * OpenCLTypeStruct TypeTable[]
+//Type information for return types and arguments.
+//
+//  * unsigned SignatureTable[]
+//A list of types representing function signatures.  Each entry is an index
+//into the above TypeTable.  Multiple entries following each other form a
+//signature, where the first entry is the return type and subsequent
+//entries are the argument types.
+//
+//  * OpenCLBuiltinStruct BuiltinTable[]
+//Each entry represents one overload of an OpenCL builtin function and
+//consists of an index into the SignatureTable and the number of arguments.
+//
+//  * std::pair isOpenCLBuiltin(llvm::StringRef Name)
+//Find out whether a string matches an existing OpenCL builtin function
+//name and return an index into BuiltinTable and the number of overloads.
+//
+//  * void OCL2Qual(ASTContext&, OpenCLTypeStruct, std::vector&)
+//Convert an OpenCLTypeStruct type to a list of QualType instances.
+//One OpenCLTypeStruct can represent multiple types, primarily when using
+//GenTypes.
+//
 //===--===//
 
 #include "llvm/ADT/MapVector.h"
@@ -57,34 +84,52 @@
   // The output file.
   raw_ostream 
 
-  // Emit the enums and structs.
+  // Helper function for BuiltinNameEmitter::EmitDeclarations.  Generate enum
+  // definitions in the Output string parameter, and save their Record instance
+  // in the List parameter.
+  // \param Types (in) List containing the Types to extract.
+  // \param TypesSeen (out) List containing the Types already extracted.
+  // \param Output (out) String containing the enums to emit in the output file.
+  // \param List (out) List to fill with the extracted types.
+  void ExtractEnumTypes(std::vector ,
+StringMap , std::string ,
+std::vector );
+
+  // Emit the enum or struct used in the generated file.
+  // Populate the TypeList at the same time.
   void EmitDeclarations();
 
-  // Parse the Records generated by TableGen and populate OverloadInfo and
-  // SignatureSet.
+  // Parse the Records generated by TableGen to populate the SignaturesList,
+  // FctOverloadMap and TypeMap.
   void GetOverloads();
 
-  // Emit the OpenCLSignature table. This table contains all possible
-  // signatures, and is a struct OpenCLType. A signature is composed of a
-  // return type (mandatory), followed by zero or more argument types.
+  // Emit the TypeTable. This table contains all the possible types. A type can
+  // have several attributes (e.g. vector size, whether it is a pointer type,
+  // constness, ...).
+  // For example
+  //   {OCLT_Double, 2},
+  // is describing the type "Double", being a vector of 2 elements. See the
+  // struct OpenCLTypeStruct for more information.
+  void