This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add image type handling for builtin functions
ClosedPublic

Authored by svenvh on Jun 18 2019, 2:47 AM.

Details

Summary

Image types were previously available, but not working. This patch
adds handling for OpenCL builtin functions.

Rename the image type definitions in the .td file to make them
consistent with other type names. Use abstract types to represent the
unqualified types. Instantiate access-qualified image types at the
point of use using e.g. ImageType<Image2d, "RO">.

Add/update some TableGen definitions that use image types.

Diff Detail

Repository
rL LLVM

Event Timeline

Pierre created this revision.Jun 18 2019, 2:47 AM
Pierre updated this revision to Diff 205396.Jun 18 2019, 10:40 AM
Pierre added projects: Restricted Project, Restricted Project.

Add context lines

This patch adds handling for OpenCL builtin functions.

I think this is a copy-paste from other reviews. :)

clang/lib/Sema/OpenCLBuiltins.td
44 ↗(On Diff #205396)

Btw _IsAbstract can be bit type?

136 ↗(On Diff #205396)

Btw do we always need to set all the inherited fields in each subclass in the same way?

For example VecWidth should be set to 0 in image types?

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
247 ↗(On Diff #205396)

0:default -> 0:none

449 ↗(On Diff #205396)

Can you explain this better please.

463 ↗(On Diff #205396)

Also can you explain perhaps with an example what you are trying to emit...

Pierre updated this revision to Diff 207530.Jul 2 2019, 6:05 AM
Pierre marked 4 inline comments as done.
Pierre edited the summary of this revision. (Show Details)

Corrections.

clang/lib/Sema/OpenCLBuiltins.td
136 ↗(On Diff #205396)

This was meant to take the values from _Ty when default values from the Type class could have been wrong.
This might not be necessary for every field of every subclass of the Type class, but this is safer.

For example VecWidth should be set to 0 in image types?

Indeed, the two incompatible fields that I see are the VecWidth of the ImageType class, and the AccessQualifier of the VectorType.

Pierre updated this revision to Diff 207806.Jul 3 2019, 8:40 AM

Rebase on previous patches.

Pierre updated this revision to Diff 208345.Jul 8 2019, 3:12 AM
svenvh commandeered this revision.Aug 21 2019, 2:05 PM
svenvh edited reviewers, added: Pierre; removed: svenvh.
svenvh updated this revision to Diff 216480.Aug 21 2019, 2:11 PM
svenvh edited the summary of this revision. (Show Details)
  • Use StringSwitch in some places
  • Improve comments
  • clang-format
  • Use an enum instead of an unsigned to represent qualifiers.
Anastasia added inline comments.Aug 28 2019, 11:12 AM
clang/lib/Sema/OpenCLBuiltins.td
136 ↗(On Diff #216480)

Probably doesn't belong to this patch I was just wondering whether it would make sense to factor our uncommon fields like AccessQualifier from common Type?

449 ↗(On Diff #216480)

I think 3D image writes are only available in CL2.0 and the same for all read_write images. Are we guarding that?

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

some testing of CL versions would be good.

svenvh marked 3 inline comments as done.Sep 2 2019, 8:21 AM
svenvh added inline comments.
clang/lib/Sema/OpenCLBuiltins.td
136 ↗(On Diff #216480)

It might be possible, but in the end we need to set the AccessQualifier field somewhere as Type represents the entries in the final table.

449 ↗(On Diff #216480)

Not yet, that will have to be guarded by a future patch as there is no handling of versions and extension on Clang master yet. Something to add to https://reviews.llvm.org/D63504 I suppose.

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

There is no handling of OpenCL versions / extensions yet.

svenvh updated this revision to Diff 218427.Sep 3 2019, 3:30 AM

Rebase, add other image read and write builtins, add some tests.

Anastasia accepted this revision.Sep 3 2019, 8:39 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Sep 3 2019, 8:39 AM
This revision was automatically updated to reflect the committed changes.