This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Group builtin functions by prototype
ClosedPublic

Authored by svenvh on Jun 19 2019, 8:03 AM.

Details

Summary

The file generated from the tablegen file, and containing the
function definitions can be re-organised to save some memory.
Functions having the same prototypes will point to a common
list of prototypes.
Functions having only one prototype will point to a prototype
definition of another function.

This depends on:
-1, splitting opencl-c.h file: https://reviews.llvm.org/D63256/new/
-2, Adding generic types: https://reviews.llvm.org/D63434
-3, Adding const, volatile and pointer types: https://reviews.llvm.org/D63442
-4, Adding image types: https://reviews.llvm.org/D63480
-5, Adding version handling: https://reviews.llvm.org/D63504

Diff Detail

Event Timeline

Pierre created this revision.Jun 19 2019, 8:03 AM

Functions having only one prototype will point to a prototype
definition of another function.

Do you mean functions having only one prototype will point to a prototype
definition of another function with the same prototype?

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
94

This optimization generally seems reasobale. Do you have any idea how it affects the build time to generate the Tablegen file and how much space do we actually save?

202

It is not clear whether signatures for functions that don't share the prototype with any others should still be in this table?

222

This data stricture is really difficult to understand, do you think you could split it into multiple type aliases?

521

I think you need to either simplify or document what (SignatureListMap.find(Candidate)->second.first)[Index].first expression returns.

545

where is this being deallocated?

558

same problem as before... expression is too complex!

567

The same here... where do we release the memory?

Pierre updated this revision to Diff 208349.Jul 8 2019, 3:14 AM
svenvh commandeered this revision.Sep 30 2019, 8:56 AM
svenvh updated this revision to Diff 222446.
svenvh edited reviewers, added: Pierre; removed: svenvh.
svenvh edited the summary of this revision. (Show Details)
  • Improve variable names & comments.
  • Fix memory leaks.
  • Rebase on top of recent master, and move before https://reviews.llvm.org/D63555 as the generated code on master is growing in size; with this patch we save about 100kb in the binary.
  • Restucture SignatureListMap to make it easier to understand.
svenvh marked 13 inline comments as done.Sep 30 2019, 9:05 AM
svenvh added inline comments.
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
94

It doesn't affect build time in a noticable way. With this patch, clang-tblgen -gen-clang-opencl-builtins runs in less than half a second.

We save about 100kb now. The savings will increase when we add more OpenCL builtins to the .td file.

202

All signatures will go into this table. Some might be shared, some not.

222

Done.

521

Broken up (and readability benefits from the SignatureListMap restructuring too).

545

At lines 535 and 545 now.

558

Better now?

567

You were referring to a new expression here, which is now gone.

Anastasia added inline comments.Nov 1 2019, 8:41 AM
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
200

This sentence is a bit hard to understand. I am also still confused what BuiltinIndexListTy represents.

202

I am confused where does the saving come from. We are currently adding an extra data structure without removing any other. So do we then reduce number of entries in SignaturesList?

557

frame?

svenvh marked 9 inline comments as done.Nov 4 2019, 3:10 AM
svenvh added inline comments.
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
200

Would it help if it was rephrased as follows?

Map an ordered vector of signatures to their original TableGen Record instances, and to a list of function names that share these signatures.

202

When generating the tables, this patch will make EmitBuiltinTable iterate over the entries in SignatureListMap instead of FctOverloadMap. SignatureListMap essentially contains the same information as FctOverloadMap, but it results in fewer table entries as we group builtins with the same signatures together.

557

"frame" here means an index into the BuiltinTable and the number of BuiltinTable entries, which together compose a list of signatures. This is the pair being returned by the StringMatcher when a name is successfully matched.

Frankly speaking the comment is a bit confusing, will rephrase as:
// A single signature list may be used by different builtins. Return the same <index, length> pair for each of those builtins.

Anastasia accepted this revision.Nov 4 2019, 4:18 AM

LGTM! Feel free to change comments on the final commit! Thanks!

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
200

Yes, it is much more readable! Thank you!

557

Perfect! Thanks!

This revision is now accepted and ready to land.Nov 4 2019, 4:18 AM
This revision was automatically updated to reflect the committed changes.