Page MenuHomePhabricator

[OpenCL] Add support of __opencl_c_read_write_images feature macro

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



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

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


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.


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


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.

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

Thanks. I've submitted the patch: Please approve.