Page MenuHomePhabricator

[OpenCL] Add version handling and add vector ld/st builtins
ClosedPublic

Authored by svenvh on Jun 18 2019, 10:38 AM.

Details

Summary

Allow setting a MinVersion, stating from which OpenCL version a
builtin function is available, and a MaxVersion, stating from which
OpenCL version a builtin function should not be available anymore.

Guard some definitions of the "work-item" builtin functions according
to the OpenCL versions from which they are available.

Add the "vector data load and store" builtin functions (e.g. vload/vstore),
whose signatures differ before and after OpenCL 2.0 in the pointer
argument address spaces.

Patch by Pierre Gondois and Sven van Haastregt.

Diff Detail

Repository
rL LLVM

Event Timeline

Pierre created this revision.Jun 18 2019, 10:38 AM
Anastasia added inline comments.Jul 1 2019, 8:02 AM
clang/lib/Sema/OpenCLBuiltins.td
183 ↗(On Diff #205393)

Would it make sense to rename this to CLAny or CLAll instead? Although do we even need to add a name for this or may be rather keep as numeric value representing default initialization?

308 ↗(On Diff #205393)

Table 15 from?

clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
101 ↗(On Diff #205393)

This should also work in CL2 right?

Pierre updated this revision to Diff 207349.Jul 1 2019, 9:21 AM
Pierre marked 4 inline comments as done.

Corrections on names, wrong OpenCL versions in comment, missing comma ...

clang/lib/Sema/OpenCLBuiltins.td
183 ↗(On Diff #205393)

Indeed, it makes more sense to rename it CLAny or CLAll. It also might be possible not to define this version (and not to give a name). I would prefer to use a name though. I changed it to CLAll for now.

308 ↗(On Diff #205393)

This is from OpenCL v2.0 s6.13.7. This was just a hint to help find back the definition in the specification. I didn't want to have too many ornaments around these hints. Should I give the exact chapter each time?

clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
101 ↗(On Diff #205393)

This is correct, I removed the condition.

Pierre updated this revision to Diff 207811.Jul 3 2019, 9:02 AM

Rebase on previous patches.

Pierre updated this revision to Diff 208347.Jul 8 2019, 3:12 AM
svenvh commandeered this revision.Sep 6 2019, 10:20 AM
svenvh edited reviewers, added: Pierre; removed: svenvh.
svenvh updated this revision to Diff 219136.Sep 6 2019, 10:26 AM
svenvh retitled this revision from [OpenCL] Add version handling for builtin functions to [OpenCL] Add version handling and add vector ld/st builtins.
svenvh edited the summary of this revision. (Show Details)

Rebasing and improving comments.

Anastasia accepted this revision.Sep 13 2019, 3:56 AM

LGTM! Thanks!

clang/lib/Sema/OpenCLBuiltins.td
395 ↗(On Diff #219136)

I wish TableGen had templates :)

572 ↗(On Diff #219136)

Some of these image BIFs are also conditional on CL version. Do you plan it in a separate patch?

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
124 ↗(On Diff #219136)

Is 891 relevant here?

This revision is now accepted and ready to land.Sep 13 2019, 3:56 AM
svenvh marked 2 inline comments as done.Sep 18 2019, 2:22 AM
svenvh added inline comments.
clang/lib/Sema/OpenCLBuiltins.td
572 ↗(On Diff #219136)

There is some followup work for this needed indeed. I believe that all image builtins that you are thinking of in your comment, take an image type that is only available in specific OpenCL versions. For example the image builtins taking a read_write image will be declared in CL1.1 mode currently, but they won't be accessible for any input programs because the read_write image types are unavailable in CL1.1 mode.

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
124 ↗(On Diff #219136)

Not in particular (it is the index of the table entry), but this brings the comment in sync with what is being generated. The change on this line should have been part of r369253 actually.

This revision was automatically updated to reflect the committed changes.