Page MenuHomePhabricator

[OpenCL] Add support of __opencl_c_read_write_images feature macro
ClosedPublic

Authored by azabaznov on Jun 25 2021, 7:04 AM.

Details

Summary

This feature requires support of __opencl_c_images, so diagnostics for that is provided as well

Diff Detail

Event Timeline

azabaznov created this revision.Jun 25 2021, 7:04 AM
azabaznov requested review of this revision.Jun 25 2021, 7:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 7:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Anastasia added inline comments.Jun 29 2021, 2:58 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10114–10115

Suggesting a slight rewording:
prior to OpenCL C version 2.0 or in version 3.0 without __opencl_c_read_write_images

clang/lib/Basic/Targets.cpp
746

Maybe we should change this comment to something like:

// Validate the feature dependencies for OpenCL C 3.0.

Since it is not exactly about the macros.

752

Maybe we should comment that the first element of the pair is the feature that depends on the feature in the second element?

757

I guess we don't need the compound assignment here?

azabaznov updated this revision to Diff 355853.Jul 1 2021, 6:15 AM

Addressed latest review comments, also refactored the patch by putting diagnostics into OpenCLOptions

azabaznov marked 3 inline comments as done.Jul 1 2021, 6:16 AM
azabaznov updated this revision to Diff 355890.Jul 1 2021, 8:13 AM

Fixed naming of new methods, fixed comments, removed redundant comments from source file

Anastasia accepted this revision.Jul 12 2021, 4:15 AM

LGTM! Thanks

This revision is now accepted and ready to land.Jul 12 2021, 4:15 AM
thakis added a subscriber: thakis.Jul 13 2021, 6:31 AM
thakis added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
7412

Are the parens right here? You probably want

`!S.getLangOpts().OpenCLCPlusPlus && (Ver < 200 || (Ver == 300 && isSupported))`

for the first term, but you have

`!S.getLangOpts().OpenCLCPlusPlus && (Ver < 200) || (Ver == 300 && isSupported)`

Which means the OpenCLCPlusPlus only matters for (Ver < 200) and the (Ver == 300 && ...) term makes things true independent of the OpenCLCPlusPlus check.

If what you currently have is what you want: Remove the parens around (S.getLangOpts().OpenCLVersion < 200) and do instead parenthesize like so: (!S.getLangOpts().OpenCLCPlusPlus && Ver < 200).

azabaznov added inline comments.Jul 13 2021, 6:54 AM
clang/lib/Sema/SemaDeclAttr.cpp
7412

Thanks. I've submitted the patch: https://reviews.llvm.org/D105892. Please approve.