Page MenuHomePhabricator

[OpenCL] Add generic type handling for builtin functions
AbandonedPublic

Authored by svenvh on Jun 17 2019, 8:32 AM.

Details

Reviewers
Anastasia
Pierre
Summary

Generic types are an abstraction of type sets. It mimics the way functions
are defined in the OpenCL specification.
E.g.: 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; reduce the memory impact of functions with numerous prototypes.

This patch depends on https://reviews.llvm.org/D63256/new/

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Anastasia added inline comments.Jun 21 2019, 6:52 AM
clang/lib/Sema/OpenCLBuiltins.td
49

What does abstract type mean here?

clang/lib/Sema/SemaLookup.cpp
738

should this be a static function?

738

How about SortOpenCLBuiltinFctTypes? Also does it do any sorting? If not may be it should be GetOpenCLBuiltinFctOverloads?

775

Can we rename this into InsertOCLBuiltinDeclarationsFromTable

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
64

do we do any sorting?

95

references -> represents?

96

reference -> represent?

98

reference -> represent?

237

Why is this being changed? Does it relate to GenericType?

245

I just realized that we don't describe at a high level what this code generates. May be it's good to add a comment that describes the general structure of generated code.

It can be done as a separate patch though but I find it hard to review without understanding the overall structure.

385

Can you document what this function does? I can't quite get why it returns a vector.

404

Can you add a comment here:
// Start of switch statement over all possible types used in Builtin functions

443

Does abstract mean generic type?

455

end of switch statement?

461

I can't understand what this code does...

Pierre updated this revision to Diff 206682.Jun 26 2019, 8:38 AM
Pierre marked 35 inline comments as done.
Pierre added inline comments.
clang/lib/Sema/OpenCLBuiltins.td
49

This is to compare with the CanQualType instances in ASTContext.h (and BuiltinTypes.def and OpenCLImageTypes.def).
Some types in this .td file don't represent a real type, as defined in the files referenced above. This is the case for the generic types, representing multiple types and vector size, but also for the image types in D63480.
This isAbstract field is meant to identify the Type instances (of the .td file) that cannot be mapped to an existing ASTContext CanQualType.

For a function using the float Type in the .td file, this type will generate the following code in OpenCLBuiltins.inc (see line 443 of ClangOpenCLBuiltinEmitter.cpp):

case OCLT_float:
  QT.push_back(Context.FloatTy);
  break;

For a function using the generic type FGenTypeN , this type will generate the following code:

case OCLT_FGenTypeN:
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  GenTypePossibility = 3;
  GenVectorPossibility = &ListListN;
  break;

GenTypePossibility and GenVectorPossibility can be ignored for now. FGenTypeN cannot be mapped to one CanQualType.
It is currently possible to identify instances of the GenericType class for now (and remove the IsAbstract field), but some instances of the futur-defined image types will cause troubles. Indeed, some Type instances will have to be ignored, and this is a way to select which types to ignore.

63

The child classes are VectorType, PointerType, ImageType and GenericType, I don't get what is outdated?

64

This means that VectorType classes should only be defined when used the definition of a builtin function, and not as below:

def int2_t : VectorType<int_t, 2>;

I changed it.

162

In the OpenCL C specification, vector types are defined just after the non-vector types. Having this comment here makes it easier to find than having it in the definition of one builtin function (I think)

221

No reason, this has been changed

clang/lib/Sema/SemaLookup.cpp
675

I think this is of

681

A generic type can abstract multiple types, cf the definition of fGenTypeN in the .td file line 221. In this case, GenTypeMaxCnt's value will be 3

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
64

I changed to "extract"

95

I used reference because the numbers are indexes in another table (the TypeTable)

96

Same

98

I changed to "represent"

237

I removed the enum. The "0 otherwise" is important because generic types can have different vector sizes, but the vector sizes of generic types are not handled with this field: this field is irrelevant for generic types.

245

I added a paragraph in the header of this file. I am not sure this was what you wanted.

443

See comment l.49 of OpenCLBuiltins.td

461

(l.459) TypeList contains all the types, excepted generic types.
If the type currently handled is a non-generic type, then its VectorWidth field in the TypeTable will be set to its vectorwidth, and the code below (line 476) will set its vectorwidth.
If this is a generic type, the VectorWidth field in the TypeTable is not relevant. The vectorwidth of the available types will be set with the code line 461.

For the generic type FGenTypeN, the types float, double and half can have the following vector width: scalar, 2, 3, 4, 6, 8, 16.
We first populate the list of QualType QT with the right types (regardless the vector width).
There are three different types available, so we set GenTypePossibility to 3.
The vectorwidth we want these types to have is stored in GenVectorPossibility, referencing ListListN (={0, 2, 3, 4, 8, 16})

case OCLT_FGenTypeN:
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  GenTypePossibility = 3;
  GenVectorPossibility = &ListListN;
  break;

Then, we need to set the right vectorwidths for each element of QT. l.462 is checking this is not a scalar (GenVectorPossibility[Idx] != 0). If this is not a scalar, we set the vectorwidth to the element of GenVectorPossibility at index I/GenTypePossibility.
The order of element in QT is therefore important.

Pierre added inline comments.Jun 27 2019, 1:44 AM
clang/lib/Sema/SemaLookup.cpp
683

I will change meaningfull -> meaningful in the next correction (and the other occurrences of the same word)

Anastasia added inline comments.Jul 2 2019, 6:02 AM
clang/lib/Sema/OpenCLBuiltins.td
49

Ok, can you update the comment by saying something like that the abstract types cannot be mapped to any CanQualType...

49
case OCLT_FGenTypeN:
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  QT.push_back(Context.FloatTy);
  QT.push_back(Context.DoubleTy);
  QT.push_back(Context.HalfTy);
  GenTypePossibility = 3;
  GenVectorPossibility = &ListListN;
  break;

Hmm it seems you are repeatedly adding the same thing.

QT.push_back(Context.FloatTy);
QT.push_back(Context.DoubleTy);
QT.push_back(Context.HalfTy);
63

Just add etc. at the end in case we add extra types later and this comment will become outdated.

118

Possible types -> Possible element types

162

Well the code doesn't really have sections to refer to. So how do I find the section you mention? In fact right now if I search for 6.13.7 I don't find anything.

It is not common to refer to sections in the code because it doesn't have structural documentation as spec would have. Does it make sense?

163

You now have 2.0 twice!

171

why are you changing this?

206

What does 0 mean here? And why do we need his list?

208

Why some names start with capital letter and some not?

clang/lib/Sema/SemaLookup.cpp
675

Possibly both fit. For would read nicer because you would avoid repetition of of article. :p

676

Sorry it might be better to change to:

of a function -> for OpenCL library functions

728

for the OpenCL library functions.

763

I think you need to start from something like - Helper function ...

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
33

does this include GenType?

37

Can you explain this a bit more please?

What is the index here? You probably need to copy some similar description as below.

42

This included GenTypes right? Btw can we have GenTypes and concrete VectorTypes here too?

46

Can you add a high level description please?

52

What do the numbers correspond to?

59

what are the 2 values used for?

173

Ok, how about we add a section describing how all these containers relate to each other?
Just to make clear why we have SignaturesList and FctOverloadMap that both contain signatures of functions but yet they are different data structures.

Feel free to use type aliases for something like std::pair<const Record *, unsigned> or std::pair<std::vector<Record *>, unsigned> that might help understanding the purpose of different data structures you define.

302

Does the struct contain just one field?

309

SignTableIndex -> SigTableIndex?

Otherwise it reads as sign.

336

I think type aliases would make this more concise and clean.

529

What is GenVectorPossibility and GenTypePossibility?

540

Can you document somewhere the structure of OCL2Qual. I am not getting here are we inside or outside of switch statement...

Pierre updated this revision to Diff 207539.Jul 2 2019, 6:21 AM

Add examples of generated code.

Pierre updated this revision to Diff 207790.Jul 3 2019, 7:33 AM
Pierre marked 51 inline comments as done.

Corrections.

clang/lib/Sema/OpenCLBuiltins.td
49

Yes,

QT.push_back(Context.FloatTy);
QT.push_back(Context.DoubleTy);
QT.push_back(Context.HalfTy);

is added the same number of times as the number of elements in ListListN.

It is still possible to create for loops, but I thought it would be slightly more easier to understand this than for loops (at first sight).
The cost of having for loops instead of raw push_back like this will be similar for generic types with few elements. I think the overall cost in size of generated code is slightly worse with for loops.

63

Actually, its child classes are not concrete types, cf the GenType. I changed it to something else.

162

I changed it to something less specific.

171

These type definitions need to be excluded by being declared as abstract not to cause trouble. They will need to be abstract to be handled in the image type patch. This also allows to keep the builtin functions using image types as such, and with no harm.

206

What does 0 mean here?

0 was for a scalar. I changed it to 1.

And why do we need his list?

This list is needed in the definition of the fmax and fmin functions. The GenType_float_ListNNonZero for instance is used and uses it.

208

I changed to CamelCase wherever this was possible. The image types and address spaces qualifiers have not been changed in this patch, ut this will be in the appropriate patches.

clang/lib/Sema/SemaLookup.cpp
676

So in the end I kept "of" then "for" to avoid repetition :D

763

InsertOCLBuiltinDeclarationsFromTable will also uses a few helper functions. In the end we will only have helper functions, and I think this is actually the only real function of the -fdeclare-opencl-... feature

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
33

Yes, GenTypes are also hold inside this structure.

37

This was deleted because the struct was unnecessary (replaced by an array of unsigned)

42

Yes, GenTypes are also held inside this structure, but if this is a genType, the "VectorWidth" field is not relevant.

I added a comment // Size of the vector (if applicable, 0 otherwise and for generic types). in the definition of the structure. I didn't go into details in the outline of the structure of the file.

46

I added a more high level description just above while describing the struct itself.
cf

// struct OpenCLTypeStruct {
//   Hold information about types (vector size, ...)
// };

In this part of the file, outlining the structure of the generated output, the first sections are enum/structure definitions, then comes the instances of the structures defines in the first section, and finally functions.

52

They are indexes in the struct OpenCLTypeStruct TypeTable.

For instance, if
{OCLT_double, 2} were at index 0,
{OCLT_int, 2} were at index 2,
{OCLT_float, 1} were at index 15,

then 0, 2, 15 would be a the signature:
double2 function( int2, float) ;

59

They are referencing the SignatureTable, cf the comment on the right of the values

173

I can typedef the Record * to a name showing a bit more what is inside the Record *. For instance:
typedef Record * Type if the Record is hiding a Type, or
typedef Record * Signature if this is a signature.

I agree this is currently not explicit at all, but aliasing the whole structures can be even more misleading since all these structure represent really similar things. I will do the aliasing it in the next version.

302

Yes, it was. I changed it to an array of unsigned.

336

cf comment on line 373.

455

It was at :
} // switch (Ty.ID)
I added another comment.

529

GenTypePossibility is the number of different types held in the GenType. If the gentype contains float/half, then GenTypePossibility = 2
GenTypePossibility is a pointer to the vector sizes available for this GenType. If the is ListListN (= {1, 2, 3, 4, 8, 16, }), then these vector sizes will be generated for the types held in the Generic Type.

I added comments with generated code to be more explicit.

540

I added a description in the description of the generated code outline (beginning of the file).
I also refactored one part of the function to make it a bit clearer.

Pierre updated this revision to Diff 207798.Jul 3 2019, 8:03 AM

Forgot one change.

Pierre updated this revision to Diff 208343.Jul 8 2019, 3:10 AM

It seems Phabricator got a bit confused as the latest diff did not include full context (use git diff -U999999).

clang/lib/Sema/OpenCLBuiltins.td
65

customed -> custom

247

Any reason to change indentation/line breaks here? The previous version looked easier to read imho.

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
93

I'm not sure if having large chunks of commented out code is a good idea. It gets outdated easily, only leading to confusion. If anyone is interested to see the details of the generated code, they can always look in the generated file. I think it would be better to only describe the high level structure and the reason for generating the code in the chosen way.

540

Again I feel this is too much code that might get outdated. It would be better to describe *why* it's generated this way because anyone can already see *what* is generated in the .inc file.

svenvh commandeered this revision.Thu, Aug 1, 1:44 AM
svenvh abandoned this revision.
svenvh edited reviewers, added: Pierre; removed: svenvh.
svenvh marked 2 inline comments as done.