Page MenuHomePhabricator

[OpenCL] Introduce new language options for OpenCL keywords.
ClosedPublic

Authored by azabaznov on Feb 1 2021, 2:45 AM.

Details

Summary

OpenCL keywords 'pipe' and 'generic' are unconditionally
supported for OpenCL C 2.0 or in OpenCL C++ mode. In OpenCL C 3.0
these keywords are available if corresponding optional core
feature is supported.

Diff Detail

Event Timeline

azabaznov created this revision.Feb 1 2021, 2:45 AM
azabaznov requested review of this revision.Feb 1 2021, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 2:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
svenvh added a comment.Feb 2 2021, 2:30 AM

LGTM, but perhaps you can add a test that has each keyword disabled?

LGTM, but perhaps you can add a test that has each keyword disabled?

FYI we currently already test that pipe and generic are valid for OpenCL 2.0 and invalid for OpenCL < 2.0. Or do you mean different kind of tests? In OpenCL 3.0 we will have to set the new LangOpts fields based on the values of OpenCLOptions but my guess is that is going to be added in the subsequent patches...

clang/include/clang/Basic/LangOptions.def
218

Normally we use just a name of the feature, so perhaps:

OpenCLGenericKeyword -> OpenCLGenericAddressSpace
OpenCLPipeKeyword -> OpenCLPipe

LGTM, but perhaps you can add a test that has each keyword disabled?

FYI we currently already test that pipe and generic are valid for OpenCL 2.0 and invalid for OpenCL < 2.0. Or do you mean different kind of tests? In OpenCL 3.0 we will have to set the new LangOpts fields based on the values of OpenCLOptions but my guess is that is going to be added in the subsequent patches...

Yes, I believe there are tests already for earlier versions. Should I mark this change as NFC?

azabaznov added inline comments.Feb 2 2021, 7:23 AM
clang/include/clang/Basic/LangOptions.def
218

Ok, will change.

svenvh added a comment.Feb 2 2021, 8:06 AM

LGTM, but perhaps you can add a test that has each keyword disabled?

FYI we currently already test that pipe and generic are valid for OpenCL 2.0 and invalid for OpenCL < 2.0. Or do you mean different kind of tests? In OpenCL 3.0 we will have to set the new LangOpts fields based on the values of OpenCLOptions but my guess is that is going to be added in the subsequent patches...

Actually the existing tests should cover this indeed. I was thinking of tests for the OpenCL 3 case, but that is probably something for a future patch.

LGTM, but perhaps you can add a test that has each keyword disabled?

FYI we currently already test that pipe and generic are valid for OpenCL 2.0 and invalid for OpenCL < 2.0. Or do you mean different kind of tests? In OpenCL 3.0 we will have to set the new LangOpts fields based on the values of OpenCLOptions but my guess is that is going to be added in the subsequent patches...

Yes, I believe there are tests already for earlier versions. Should I mark this change as NFC?

I am not sure since we are adding a new LangOpts entries it's not entirely NFC. I would just live as is.

azabaznov updated this revision to Diff 321348.Feb 4 2021, 1:45 AM

Renamed language options and adjusted comment.

azabaznov marked an inline comment as done.Feb 4 2021, 1:46 AM
svenvh accepted this revision.Feb 4 2021, 2:26 AM

LGTM!

This revision is now accepted and ready to land.Feb 4 2021, 2:26 AM
Anastasia accepted this revision.Feb 4 2021, 2:57 AM

LGTM! Thanks

This revision was landed with ongoing or failed builds.Feb 5 2021, 12:19 AM
This revision was automatically updated to reflect the committed changes.