[PATCH] D60763: Prototype OpenCL BIFs using Tablegen
Anastasia Stulova via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 26 21:04:58 PDT 2019
Anastasia marked an inline comment as done.
Anastasia added a comment.
In D60763#1478672 <https://reviews.llvm.org/D60763#1478672>, @Pierre wrote:
> I also think we could reduce the size of the tables.
> To sum up how this is working right now:
>
> 1. The isOpenCLBuiltin(char* functionName) funcion is called to determine if a function is part of OpenCL builtin functions. If so, it returns its associated pair (index, number of signatures) in the OpenCLBuiltinDecl, otherwise it returns 0.
> 2. The OpenCLBuiltinDecl table is storing, for each OpenCL builtin function, the list of the possible signature associated for this function (e.g. cos can be "float cos(float)", "double cos(double)", ...). The available signatures are stored in the OpenCLSignature table. In the OpenCLBuiltinDecl are stored the indexes corresponding to the possible signature for each function.
> 3. The OpenCLSignature is storing the possible signatures.
>
> E.g.: For the prototype float cos(float):
>
> 1. isOpenCLBuiltin("cos") is returning the pair (345, 18)
> 2. OpenCLBuiltinDecl[345] to OpenCLBuiltinDecl[345+18] are the available signatures of the builtin function "cos". Let say OpenCLBuiltinDecl[346] is containing our "float cos(float)" prototype. OpenCLBuiltinDecl[346] is containing the pair (123, 2), indexing the OpenCLSignature table and how many entries we have to read.
> 3. OpenCLSignature[123] is storing the return type of the function, so the "float" type. OpenCLSignature[124] is containing the type of the first argument, so the float type again. We are not looking further in the table because we are only looking for 2 types.
>
> ---------------------------------------------------------------------------------------
>
> In the "float cos(float)" prototype, the information about the "float" type is duplicated. Plus, this "float" type is also the same as in the "float sin(float)" function. A third table, storing the different types, would discard duplicated definitions of types. The OpenCLSignature would store indexes of the required types, and the third table the type itself. This third table would be called OpenCLTypes, and would be as:
>
> ``` struct OpenCLTypes { // A type (e.g.: float, int, ...) OpenCLTypeID ID; // Size of the vector (if applicable) unsigned VectorWidth; // 0 if the type is not a pointer unsigned isPointer; // Address space of the pointer (if applicable) clang::LangAS AS; } ``` and OpenCLSignature:
>
> ``` struct OpenCLSignature { unsigned OpenCLTypesIndex } ``` --------------------------------------------------------------------------------------- Another way to save space would be to group functions. The "sin" and "cos" functions are similar (identical, we could say), regarding their use/ signature. However, they have two distinct lists of signatures in the OpenCLBuiltinDecl table. The consequence would be that isOpenCLBuiltin("cos") and isOpenCLBuiltin("sin") would give the same output. Such group of functions could be created manually by adding an attribute in the OpenCLBuiltins.td file, or automatically generated in the ClangOpenCLBuiltinEmitter.cpp file. The first solution would however make the feature potentially less understandable/ more complex to add new functions. The second solution is complex to implement/ could require a lot of time to process.
I think it would be a good idea to investigate explore the solution you are proposing, but I suggest to do it once we add more functionality so we can benchmark a bit more.
Btw, can you upload a full diff next time please.
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:11
+//
+// The file is organized as:
+// -Base definitions: base classes/ definitions required in sections below
----------------
I would rather use the header comment to just describe what the purpose and overall functionality of this file is at a high level. There is a danger of structures to get outdated quite quickly.
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:48
+// 4 :: opencl_generic
+// (Following address spaces are not available for OpenCL)
+// 5 :: cuda_device
----------------
I think we don't have to say what ASes are not supported, they will evolve and this will become outdated anyway.
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:63
+
+// Qualified Type. Allow to retrieve one ASTContext Qualtype.
+class QualType<string _AccessMethod,string _Name> {
----------------
Qualtype -> QualType
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:65
+class QualType<string _AccessMethod,string _Name> {
+ // How to get the QualType. Can be one of ("field", "func")
+ string AccessMethod = _AccessMethod;
----------------
I don't think "field" and "func" are clear at this point.
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:99
+ int HasSat = 0;
+ // Function arguments can have ab access qualifiers (read_only, ...). Can be
+ // one of ("RO", "WO", "RW")
----------------
ab - > an
I would say here something like - some types can have an access qualifier...
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:211-212
+ def float_t : Type<"float", QualType<"field", "FloatTy">>;
+ def double_t : Type<"double", QualType<"field", "DoubleTy">>;
+ def half_t : Type<"half", QualType<"field", "HalfTy">>;
+}
----------------
Nicola wrote:
> Can half and double types and builtins be made dependent on extensions or configurable? The half datatype needs the cl_khr_fp16 extension, while double needs CL_DEVICE_DOUBLE_FP_CONFIG or cl_khr_fp64
half and double types are already activated by an extension in Clang. This behavior isn't modified here.
As for builtins there is `Extension` field in Builtin, but I think it's not used in conversion functions at the moment. I guess we should update that.
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:246
+def image1d_array_t : Type<"image1d_array_t", QualType<"null", "null">>;
+// These definitions are used: the QualType is defined.
+foreach v = ["RO", "WO", "RW"] in {
----------------
Can you clarify this comment please?
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:326
+// example 'foo', to show using 'version'
+def : Builtin<"foo_version", [int_t, PointerType<int_t, global_as>]>;
+let Version = CL20 in {
----------------
I think we now need to use some real function here
================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:331
+
+// Example showing 'Extension'
+let Extension = "cl_khr_subgroups" in {
----------------
I think this is no longer an example so we can change this something like Built-in Subroups Extension...
Also this should probably moved to the bottom.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:675
+static void InsertOCLBuiltinDeclarations(Sema &S,
+ LookupResult &LR,
----------------
this should be documented - at least we should say what this function can be used for.
But I think parameters are not entirely clear too.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:680
+ unsigned Len) {
+ // Once it is known the identifier is referencing an OpenCL builting function,
+ // add all its prototypes to the LookUpResult.
----------------
'it' is not clear here.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:694
+
+ QualType RT = OCL2Qual(Context, OpenCLSignature[Decl.ArgTableIndex]);
+
----------------
I think we should explain where this function definition comes from.
================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:9
+
+kernel void test(global float4* buf, global int4* res)
+{
----------------
I would suggest to name each function based on what it's intended to test i.e. common, version, extensions...
In the future we might partition it even further based on builtin function kind: convert, math, etc... but perhaps we can think about it later.
================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:15
+kernel void test2(global int* bar) {
+ bar[0] = foo_version(bar);
+}
----------------
this isn't a valid builtin
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:14
+//
+//===----------------------------------------------------------------------===//
+
----------------
We should explain the overall concept i.e. this class contains functionality to emit builtin functions in a some format. Explanation of the format can probably be done inline with class members...
We should also say where and how the generated code is used.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:41
+private:
+ RecordKeeper &Records;
+ raw_ostream &OS;
----------------
What is the `Record` here?
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:44
+
+ void EmitDeclarations();
+ void GetOverloads();
----------------
I think this class should be documented better to describe what it generates and in what format inline with each member. See some comments below.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:51
+
+ // Contains a list of the available signatures, regardless the name of the
+ // function. Each pair consists in a signature and a cumulative index.
----------------
regardless -> without ?
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:52
+ // Contains a list of the available signatures, regardless the name of the
+ // function. Each pair consists in a signature and a cumulative index.
+ // E.g.: <<float, float>, 0>,
----------------
consists in -> consists of
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:58
+ // <<double, double>, 35>.
+ std::vector<std::pair<std::vector<Record *>, unsigned>> SignatureSet;
+
----------------
I think it's worth checking whether inner vector is a good candidate for using SmallVector of llvm. We can probably just use it with size 3 by default since size is normally between 1-3. Although we could as well add a TODO and address it later if we encounter performance problems.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:61
+ // Map the name of a builtin function to its signatures.
+ // Each signature is registered as a pair of:
+ // <pointer to a list of types (a signature),
----------------
Why not to just refer to the data structure above or we can alternatively just typedef the vector?
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:75
+void BuiltinNameEmitter::Emit() {
+ // Generate the file containing OpenCL builtin functions checking code.
+
----------------
I think this comment belong to above.
Actually let's document all methods in a class definition consistently.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:96
+void BuiltinNameEmitter::EmitDeclarations() {
+ // Emit the enum/ struct used in the file generated
+
----------------
The same - can be moved about the function definition.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:139
+void BuiltinNameEmitter::GetOverloads() {
+ // Parse the Records generated by TableGen to feed the OverloadInfo and
+ // SignatureSet
----------------
This belongs to method description too.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:171
+void BuiltinNameEmitter::EmitSignatureTable() {
+ // Emit the OpenCLSignature table. This table contains all the possible
+ // signature, and is a struct OpenCLType. A signature is composed of a
----------------
-> method description
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:182
+
+ std::vector<std::vector<Record *>> Signature;
+
----------------
Is this even used?
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:202
+void BuiltinNameEmitter::EmitBuiltinTable() {
+ // Emit the OpenCLBuiltins table. This table contains all the overloads of
+ // each function, and is a struct OpenCLBuiltinDecl.
----------------
-> method description
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:231
+ // std::pair<unsigned, unsigned> isOpenCLBuiltin(llvm::StringRef name);
+ std::vector<StringMatcher::StringPair> validBuiltins;
+ unsigned CumulativeIndex = 1;
----------------
Variable name should start upper case.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:247
+
+// Return 0 if name is not a recognized OpenCL builtin, or an index
+// into a table of declarations if it is an OpenCL builtin.
----------------
I think this file needs clang-format.
================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:260
+void BuiltinNameEmitter::EmitQualTypeFinder() {
+ // Construct a function returning the clang QualType instance associated with
+ // the TableGen Record Type. The prototype of the function is:
----------------
-> method definition
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60763/new/
https://reviews.llvm.org/D60763
More information about the cfe-commits
mailing list