Page MenuHomePhabricator

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Anastasia added inline comments.Apr 26 2019, 9:04 PM
clang/include/clang/Basic/OpenCLBuiltins.td
63

Qualtype -> QualType

65

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

99

ab - > an

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

211–212

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

Can you clarify this comment please?

326

I think we now need to use some real function here

331

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

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

'it' is not clear here.

694

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

clang/test/SemaOpenCL/builtin-new.cl
9

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

this isn't a valid builtin

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.

41

What is the Record here?

44

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

regardless -> without ?

52

consists in -> consists of

58

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

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

75

I think this comment belong to above.

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

96

The same - can be moved about the function definition.

139

This belongs to method description too.

171

-> method description

182

Is this even used?

202

-> method description

231

Variable name should start upper case.

247

I think this file needs clang-format.

260

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

This field wasn't necessary and has been removed

211–212

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

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

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

331

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
31

I wrote a TODO to check this later

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
58

I added a TODO

61

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

Actually no

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

clang/include/clang/Basic/OpenCLBuiltins.td
12

-> contains definition of OpenCL builtin functions

14

-> check whether the function is an OpenCL builtin.

15

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.

18

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.

19

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

27

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

70

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

124

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

169

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

244

found in -> from

306

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

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

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
98

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
18

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
299–303

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
14

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

27

simple -> basic

244

ping!

306

Why Examples?

308

Let's remove Example please!

313

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

clang/lib/Sema/SemaLookup.cpp
679

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
16

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

29

Is this TODO still needed?

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
98

ping!

line 97 now.

108

Is this TODO still relevant?

127

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
299–303

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
108

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
17

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.

67

OpenCL/C -> OpenCL C

107

-> OpenCL C

169

-> OpenCL C

clang/test/SemaOpenCL/builtin-new.cl
19

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
299–303

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
299–303

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

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.