This is an archive of the discontinued LLVM Phabricator instance.

Prototype OpenCL BIFs using Tablegen
ClosedPublic

Authored by Pierre on Apr 16 2019, 1:43 AM.

Details

Summary

This is a re-upload of the patch from Joey GOULY, posted at: https://reviews.llvm.org/D53023 . I am re-uploading it because I will continue his work on this, and it is better if I have control on the post on phabricator.
This patch contains a prototype to generate OpenCL builtin functions with Tablegen. Not all builtin functions have been implemented. This prototype is intented to replace the use of the opencl-c.h file currently included in all OpenCL programs. To recall, this file contains contains all the overloaded builtin functions. Using clang-tblgen would allow to factorize all these function declarations and include them only if they are called.

A copy-paste from the original description:

This is the prototype for the approach that was mentioned by Anastasia in http://lists.llvm.org/pipermail/cfe-dev/2018-September/059529.html

The tablegen file describes the BIFs and all their overloads, in hopefully a concise manner.

There are 3 things generated from the OpenCLBuiltins.td file.

  1. OpenCLArgTypes[], this is a table containing all the different types of overloads. This is a separate table so it can be shared by the BIFs.
  2. OpenCLBuiltins[], this is a table that contains all the overloads for the BIFs.
  3. isOpenCLBuiltin, this is a function that uses a trie-like switch/case to determine if a StringRef is the name of a BIF.

Just a quick snippet of the above:

OpenCLType OpenCLArgTypes[] = {
// 0
{ OCLT_float, 0, 0, clang::LangAS::Default, },
// 1
{ OCLT_float, 2, 0, clang::LangAS::Default, },
OpenCLBuiltinDecl OpenCLBuiltins[] = {
// acos
  { { OCLT_float, 0, 0, clang::LangAS::Default, }, 1, 0, "", 100,  },
  { { OCLT_float, 2, 0, clang::LangAS::Default, }, 1, 1, "", 100,  },
std::pair<unsigned, unsigned> isOpenCLBuiltin(llvm::StringRef name) {
  switch (name.size()) {
  default: break;
  case 3:  // 1 string to match.
    if (memcmp(name.data()+0, "foo", 3) != 0)
      break;
    return std::make_pair(707, 2);   // "foo"
}

While it's a prototype, I have tried to keep it as clean as possible.

TODO:

  1. Bit-pack the tables to reduce the size.
  2. Include the return type in the ArgTypes table to reduce the size.
  3. Measure the performance / size impact
  4. Auto-generate parts of OCL2Qual, to reduce repeated typing
  5. OCL2Qual does not support pointers-to-pointers currently, but I believe no BIFs use that.
  6. InsertBuiltinDeclarations builds up an AST function declaration manually, perhaps there is a helper function for this.
  7. There is a FIXME in SemaDecl.cpp that needs to be implemented.

Diff Detail

Repository
rL LLVM

Event Timeline

Pierre created this revision.Apr 16 2019, 1:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 1:43 AM
Pierre updated this revision to Diff 195336.Apr 16 2019, 1:46 AM

Deleted blank line

Nicola added a subscriber: Nicola.Apr 16 2019, 1:56 AM
Anastasia added a subscriber: joey.Apr 16 2019, 7:23 AM

Not related to this patch but it might be good to start thinking about testing this functionality properly. In the past we haven't tested the header because it would take a lot of testing time. So I would suggest we keep a light minimal basic testing in regular clang tests as is, but then we either add special build target to tests extra header functionality or add special cmake option (i.e. LLVM_INCLUDE_OPENCL_BIFS_TESTS) that would enable such testing with regular check-clang. I assume the latter can even be used to avoid generating the extra tables from TableGen minimizing the impact on the clang binary size for the situations OpenCL isn't required in the installation.

Any thoughts?

clang/include/clang/Basic/OpenCLBuiltins.td
10 ↗(On Diff #195336)

I would add a comment describing each section:

  • Defining classes
  • Instantiating types
  • Defining buintin functions
  • etc...

Also I am a bit lost in the structure here. I think we should group functionality in sections + document each (as requested above).

15 ↗(On Diff #195336)

can be renamed to addrSpace

16 ↗(On Diff #195336)

why is the naming here follows different convention?

50 ↗(On Diff #195336)

may be this can go below or above?

65 ↗(On Diff #195336)

half?

82 ↗(On Diff #195336)

again the name is not coherent with the rest.

125 ↗(On Diff #195336)

I think we need a real BIF here or it has to be removed from the commit for now?

clang/lib/Sema/SemaLookup.cpp
675 ↗(On Diff #195336)

Does this mean we have to produce this in ClangOpenCLBuiltinEmitter.cpp?

762 ↗(On Diff #195336)

@joey are you suggesting to use a common helper function?

777 ↗(On Diff #195336)

Variable naming convention is not followed!

clang/test/SemaOpenCL/builtin-new.cl
15 ↗(On Diff #195336)

I guess we need to find an actual function here. How about ctz?

Although, it doesn't have to be CL2.0 actually.

30 ↗(On Diff #195336)

I think different overloads can be available in different CL version. @svenvh can we already handle this?

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
12 ↗(On Diff #195336)

Builtin -> builtin function

34 ↗(On Diff #195336)

I think this file deserves more documentation...

Pierre updated this revision to Diff 196591.Apr 25 2019, 2:48 AM
Pierre marked 10 inline comments as done.

In this new patch:

  • Documentation has been added
  • The multiclasses in OpenCLBuiltins.td filehave been slighly changed to have a more generic way to generate function prototypes
  • In ClangOpenCLBuiltinEmitter.cpp, the code of the Emit() function has been shifted to functions
  • In SemaLookUp.cpp, the OCL2Qual function, used to retrieve the QualType instance of a type, is now generated by Tablegen backend

Other comments:
1-
When a header file is included, its function declarations are decorated with the "nounwind" attribute, meaning that the function is not supposed to throw an exception. This decorator is currently not added with the new mechanism.
The "readnone" decorator is also present for these builtin functions and added somewhere y clang, but not added with the new mechanism.
This can be tested with the following command line, on an .cl file containing a function calling a builtin function (e.g.: acos):

clang -cc1 -cl-std=CL2.0 -emit-llvm -O0 -I<path_to>/llvm-project/clang/lib/Headers <your_file.cl>  [-fadd-opencl-builtins|-finclude-default-header]

2-
When the function definition is inserted, it seems to be actually just resolving the identifier for the current lookup. Calling the same builtin function multiple times currently result in multiple lookup. Maybe there is a way to add the function declaration for the scope/file, so the lookup is only performed one time for each function. This part is done around the code taken from Sema::LazilyCreateBuiltin function, and I will spend more time on it.
3-
I haven't looked at the test file yet.
4-
If you have any suggestion/comment, feel free to share.

clang/include/clang/Basic/OpenCLBuiltins.td
65 ↗(On Diff #195336)

The signature of OpenCL builtin functions taking/returning half types are not part of OpenCL by default, this is part of OpenCL 2.0 extensions (cf https://www.khronos.org/registry/OpenCL/specs/opencl-2.0-extensions.pdf)

clang/lib/Sema/SemaLookup.cpp
675 ↗(On Diff #195336)

Yes I think so, I have made a change in this way

Nicola added inline comments.Apr 25 2019, 5:04 AM
clang/include/clang/Basic/OpenCLBuiltins.td
140–144 ↗(On Diff #196591)

I think the CL standard (1.2, section 6.2.3.2) specifies that RoundingModes should be available on all convert_ functions, not only selectively based on types: "Conversions may have an optional rounding mode modifier described in table 6.6. "

211–212 ↗(On Diff #196591)

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

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.

Anastasia marked an inline comment as done.Apr 26 2019, 9:04 PM

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.

clang/include/clang/Basic/OpenCLBuiltins.td
11 ↗(On Diff #196591)

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.

48 ↗(On Diff #196591)

I think we don't have to say what ASes are not supported, they will evolve and this will become outdated anyway.

Anastasia added inline comments.Apr 26 2019, 9:04 PM
clang/include/clang/Basic/OpenCLBuiltins.td
63 ↗(On Diff #196591)

Qualtype -> QualType

65 ↗(On Diff #196591)

I don't think "field" and "func" are clear at this point.

99 ↗(On Diff #196591)

ab - > an

I would say here something like - some types can have an access qualifier...

211–212 ↗(On Diff #196591)

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.

246 ↗(On Diff #196591)

Can you clarify this comment please?

326 ↗(On Diff #196591)

I think we now need to use some real function here

331 ↗(On Diff #196591)

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.

clang/lib/Sema/SemaLookup.cpp
675 ↗(On Diff #196591)

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.

680 ↗(On Diff #196591)

'it' is not clear here.

694 ↗(On Diff #196591)

I think we should explain where this function definition comes from.

clang/test/SemaOpenCL/builtin-new.cl
9 ↗(On Diff #196591)

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.

15 ↗(On Diff #196591)

this isn't a valid builtin

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
14 ↗(On Diff #196591)

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.

41 ↗(On Diff #196591)

What is the Record here?

44 ↗(On Diff #196591)

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.

51 ↗(On Diff #196591)

regardless -> without ?

52 ↗(On Diff #196591)

consists in -> consists of

58 ↗(On Diff #196591)

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.

61 ↗(On Diff #196591)

Why not to just refer to the data structure above or we can alternatively just typedef the vector?

75 ↗(On Diff #196591)

I think this comment belong to above.

Actually let's document all methods in a class definition consistently.

96 ↗(On Diff #196591)

The same - can be moved about the function definition.

139 ↗(On Diff #196591)

This belongs to method description too.

171 ↗(On Diff #196591)

-> method description

182 ↗(On Diff #196591)

Is this even used?

202 ↗(On Diff #196591)

-> method description

231 ↗(On Diff #196591)

Variable name should start upper case.

247 ↗(On Diff #196591)

I think this file needs clang-format.

260 ↗(On Diff #196591)

-> method definition

Pierre updated this revision to Diff 197932.May 3 2019, 2:29 AM
Pierre marked 39 inline comments as done.

Requested changes have been made in this diff. Some comments/ TODO have not been done yet. They will be done in a next patch. This first patch is meant to start introducing the feature in Clang.
This patch integrates the new command line option introduced in https://reviews.llvm.org/D60764

clang/include/clang/Basic/OpenCLBuiltins.td
65 ↗(On Diff #196591)

This field wasn't necessary and has been removed

211–212 ↗(On Diff #196591)

An "Extension" field will be added in the next patch, so it will be possible to retrieve an extension of a prototype from the types the prototype is using.
E.g.: In the definition of the prototype half cos(half), the prototype will have the extension "cl_khr_fp16" because it uses the "half" type.

This scheme is present for the "half" and "double" types, but also for the "image2d_depth_t" types (extension "image2d_depth_t") and others.

306 ↗(On Diff #196591)

I removed the TODO and let the functions with the half prototypes. The fact that prototypes using the "half" types are part of the "cl_khr_fp16" extension (same for the "double" type, and other types) .

326 ↗(On Diff #196591)

I changed it to a real function, but this should be changed anyway in the next patch

331 ↗(On Diff #196591)

I would like to let it as an example for now because there is only one function using the Extension field

clang/test/SemaOpenCL/builtin-new.cl
30 ↗(On Diff #195336)

I wrote a TODO to check this later

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
58 ↗(On Diff #196591)

I added a TODO

61 ↗(On Diff #196591)

I explained a bit more the purpose of the two fields in the comments.
SignatureSet is storing a list of <pointer to the signature, Index>. Tablegen is storing lists of types as objects that we can reference.
OverloadInfo is storing, for each function having the same name, a list of the following pair:
<A pointer to the TableGen "Builtin" instance,
the "Index" of its signature in the SignatureSet>

Thus, this "Index" value allows functions with different names to have the same signature. By signature I mean the list of types is uses (for float cos(float), the signature would be [float, float])

I am not sure I answered the question, but I don't think it is possible to merge the SignatureSet and the OverloadInfo structures

182 ↗(On Diff #196591)

Actually no

The overall structure looks very clear now! I just have a couple of nitpicks! Thank you!

clang/include/clang/Basic/OpenCLBuiltins.td
11 ↗(On Diff #197932)

-> contains definition of OpenCL builtin functions

13 ↗(On Diff #197932)

-> check whether the function is an OpenCL builtin.

14 ↗(On Diff #197932)

We should say that it doesn't contain all function from the spec i.e. we skip those that don't have overloads or that are added as Clang Builtins.

17 ↗(On Diff #197932)

Can you explain this comment please? At this point it almost feels like you are adding Clang Builtin here. May be you should refer to definition of OpenCL Builtin in this file.

18 ↗(On Diff #197932)

I am not getting this comment. May be we can just leave it out?

26 ↗(On Diff #197932)

I feel this comment is too abstract. May be you want to say something like - definitions of misc basic entities?

69 ↗(On Diff #197932)

Btw there are classes above too! May be this could be a section about Classes of OpenCL/C type?

123 ↗(On Diff #197932)

-> Multiclass definitions of functions with various number of parameters?

168 ↗(On Diff #197932)

This is also too abstract. How about - Definitions of builtin functions?

243 ↗(On Diff #197932)

found in -> from

305 ↗(On Diff #197932)

Why examples? I think at this point we shouldn't have examples as this is not a prototype. Also you seem to be using real function, so just remove the Examples. :)

clang/lib/Sema/SemaLookup.cpp
679 ↗(On Diff #197932)

This does not match Clang documentation format. Can you please align with other functions in this file.

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
97 ↗(On Diff #197932)

Should we remove the TODO?

Pierre updated this revision to Diff 199998.May 17 2019, 2:18 AM
Pierre marked 14 inline comments as done.

Corrections from the comments on the previous version.

clang/include/clang/Basic/OpenCLBuiltins.td
17 ↗(On Diff #197932)

I deleted this comment aswell, I feel it only adds confusion

AlexeySotkin added inline comments.May 21 2019, 12:07 AM
clang/include/clang/Basic/OpenCLBuiltins.td
298–302 ↗(On Diff #199998)

It seems like there is something wrong with access qualifiers for images. I have applied this patch and tried to compile the following code:

typedef int int2 __attribute__((ext_vector_type(2)));
typedef float float4 __attribute__((ext_vector_type(4)));

void kernel k(write_only image2d_t image, int2 coord, float4 data) {
  write_imagef(image, coord, data);
}

I got the following output:

clang -cc1 -triple spir /work/tmp/tmp.cl -emit-llvm -o -  -fadd-opencl-builtins
/work/tmp/tmp.cl:5:16: error: passing '__write_only image2d_t' to parameter of incompatible type '__read_only image2d_t'
  write_imagef(image, coord, data);
                         ^~~~~
1 error generated.
Anastasia added inline comments.May 21 2019, 3:24 AM
clang/include/clang/Basic/OpenCLBuiltins.td
13 ↗(On Diff #199998)

How about - Clang will check for functions described in this file?

26 ↗(On Diff #199998)

simple -> basic

305 ↗(On Diff #199998)

Why Examples?

307 ↗(On Diff #199998)

Let's remove Example please!

312 ↗(On Diff #199998)

Please rename Example here too! We can just say something like builting functions that belong to extensions.

243 ↗(On Diff #197932)

ping!

clang/lib/Sema/SemaLookup.cpp
679 ↗(On Diff #197932)

Ping. The format here still doesn't match the rest. Here is an example how we document functions.

6205 /// Look up the special member function that would be called by a special
6206 /// member function for a subobject of class type.
6207 ///
6208 /// \param Class The class type of the subobject.
6209 /// \param CSM The kind of special member function.
6210 /// \param FieldQuals If the subobject is a field, its cv-qualifiers.
6211 /// \param ConstRHS True if this is a copy operation with a const object
6212 ///        on its RHS, that is, if the argument to the outer special member
6213 ///        function is 'const' and this is not a field marked 'mutable'.
6214 static Sema::SpecialMemberOverloadResult lookupCallFromSpecialMember(
6215     Sema &S, CXXRecordDecl *Class, Sema::CXXSpecialMember CSM,
6216     unsigned FieldQuals, bool ConstRHS) {
clang/test/SemaOpenCL/builtin-new.cl
15 ↗(On Diff #199998)

I think we should name the functions based on what BIFs they tests. Otherwise it will become too hard to navigate and track.

28 ↗(On Diff #199998)

Is this TODO still needed?

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
97 ↗(On Diff #197932)

ping!

line 97 now.

107 ↗(On Diff #199998)

Is this TODO still relevant?

126 ↗(On Diff #199998)

This should be moved above?

Pierre marked an inline comment as done.May 21 2019, 3:30 AM
Pierre added inline comments.
clang/include/clang/Basic/OpenCLBuiltins.td
298–302 ↗(On Diff #199998)

What you are saying is right. This patch is incomplete and some features are missing/ broken.
I have a new version of the tablegen builtin feature where the access qualifiers are actually taken into account, but I cannot extract only this from my version. This would imply uploading the whole new version.
The new version will hopefully be on top of this patch, making access qualifiers work.

Pierre updated this revision to Diff 200499.May 21 2019, 7:00 AM
Pierre marked 9 inline comments as done.

The wrong patch was uploaded. Sorry for this.

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
107 ↗(On Diff #199998)

This is still relevant, but not for this patch I think.

Anastasia accepted this revision.May 23 2019, 4:36 AM

LGTM! Thanks!

Please address the minor nitpicks suggested here in your final commit.

clang/include/clang/Basic/OpenCLBuiltins.td
16 ↗(On Diff #200499)

I don't think we plan to modify opencl-c.h due to compatibility issues. I think we should factor out functionality that will be required by both TableGen mode and regular include mode into something like opencl-common.h.

66 ↗(On Diff #200499)

OpenCL/C -> OpenCL C

106 ↗(On Diff #200499)

-> OpenCL C

168 ↗(On Diff #200499)

-> OpenCL C

clang/test/SemaOpenCL/builtin-new.cl
18 ↗(On Diff #200499)

basic_extension -> basic_subgroup

This revision is now accepted and ready to land.May 23 2019, 4:36 AM
Pierre updated this revision to Diff 200961.May 23 2019, 6:47 AM
Pierre marked 4 inline comments as done.
AlexeySotkin added inline comments.May 30 2019, 1:47 AM
clang/include/clang/Basic/OpenCLBuiltins.td
298–302 ↗(On Diff #199998)

Thanks, Pierre. I'd like to start early testing of image builtins with this prototype. Do you have an idea when you will have image builtins done in this (or other) patch?
If it is not going to happen in the nearest future, would you mind if I'll propose some changes for this patch/prototype meanwhile?

svenvh added inline comments.May 30 2019, 9:34 AM
clang/include/clang/Basic/OpenCLBuiltins.td
298–302 ↗(On Diff #199998)

We're planning to commit the basic infrastructure early next week, which should make it easier to extend this work and enable contributions from the wider community.

Looks good overall, I'd like to propose a few more minor changes to this patch before landing:

  • isPointer can be a bit / bool instead of an int / unsigned.
  • Renaming the command line option from -fadd-declare-builtins to -fdeclare-opencl-builtins should hopefully convey the meaning of the option a bit better.
  • Drop the spurious ; in OCL2Qual.
  • Emit spaces instead of tabs for OCL2Qual, like the rest of the generated file.
  • Bring the format of OpenCL spec references into alignment with the rest of clang (e.g. OpenCL v2.0 s9.17.3).
  • Adding (experimental) to the help text of -fdeclare-opencl-builtins.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 2:38 AM
thakis added a subscriber: thakis.Jun 3 2019, 11:20 AM

From what I can tell, the only client of OpenCLBuiltins.td is currently lib/Sema. Do you expect that to change? If not, does it make more sense to move the .td file to there?

Do you expect OpenCLBuiltins.td to include other files in the future? At the moment, the -I ${CMAKE_CURRENT_SOURCE_DIR}/../../ bit in the cmake file isn't needed for anything.

Pierre added a comment.Jun 4 2019, 2:22 AM

From what I can tell, the only client of OpenCLBuiltins.td is currently lib/Sema. Do you expect that to change? If not, does it make more sense to move the .td file to there?

Do you expect OpenCLBuiltins.td to include other files in the future? At the moment, the -I ${CMAKE_CURRENT_SOURCE_DIR}/../../ bit in the cmake file isn't needed for anything.

OpenCLBuiltins.td is currently only targeting lib/Sema and will be moved there.
OpenCLBuiltins.td is not expected to include other files, this will be changed aswell.

Nicola added a comment.Jun 4 2019, 7:46 AM

A bit late to the review, but I've noticed a couple of issues with some of the implemented builtins:

  • The fmin/fmax builtins are defined twice for scalar types, does this create problems in overload resolution when using them?
  • The convert_ builtins don't have support for half types (which is present in the opencl-c.h header. Is that intended?
Pierre added a comment.Jun 5 2019, 2:58 AM

A bit late to the review, but I've noticed a couple of issues with some of the implemented builtins:

  • The fmin/fmax builtins are defined twice for scalar types, does this create problems in overload resolution when using them?
  • The convert_ builtins don't have support for half types (which is present in the opencl-c.h header. Is that intended?

Both your comments are rights. Some builtin functions are currently a faulty/missing. I am currently updating the functions so that they match the OpenCL C specification.
convert_half will be present, and the way builtin functions are defined in the .td file will change a bit.
The new version should be available by the end of the week.