Thanks for working on this. LGTM in general. I'll let Marco comment on the latest changes.

I see the update regaring this functionality on github issue: https://github.com/KhronosGroup/OpenCL-Docs/issues/82#issuecomment-785496025. Is it a right way to reflect this information in`OpenCLExtensions.def`?

FYI, I would even be ok if we remove the need for enabling non-core

Thanks! I review is shortly. As for now, I also was doing some refactoring: https://reviews.llvm.org/D97058. Check-clang passes, as I see there are no conflicts for now.

Thanks for review. Last change is a small fix for if clang-tidy warnings

Fix clang-tidy warnings

Rewrote 'RemoveAddressSpaceFromPtr' to return canonical type

Thanks for review. Just to be clear: can I proceed with this? Or we should discuss it first? As I see possible this change can be abandoned after discussion.

Adjusted test one more time: moved header functionality into separate test

Features remain defined in header for OpenCL C 2.0; adjusted the test

Renamed language options and adjusted comment.

In D95778#2536266, @Anastasia wrote:

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...

Rebased one more time

Rebased

Rebased

I suggest that renaming 'extension' to 'option' (extensions + features) concept to be done in separate commit. There are a lot of places to change all over the place in clang.

Due to refactoring in https://reviews.llvm.org/D92277 this flow is no longer valid. New patch for this functionality is here: https://reviews.llvm.org/D95776.

Removed getCoreFeatures() as it's not used in this change.

Thanks for feedback. I agree that providing diagnostics for unsupported core features is enough for now (I guess I'll try to come up with something for OpenCL C 3.0 features to set them unconditionally for 2.0, or at least these macros can be defined in header only). So I removed setting of core features (and also addressed all cosmetic concerns).

Changes in the latest patch:

Changes in the latest patch:

Thanks for working on this! It is a very good refactoring that brings OpenCL closer inline with convention design flow

I didn't want to end up with a big refactoring from this patch but now I start to think that considering the situation it might be unavoidable. :(

@Anastasia, there is common functionality with your approach in https://reviews.llvm.org/D91531. I guess these two patches can be unified at some point once we discuss it.

Addressed almost all technical and cosmetic concerns. Except putting reference of OpenCLOptions in Sema due to const of TargetInfo. I am going to think about that later. Also:

Hi! Great to see the interest in OpenCL C 3.0!

So if I understand it well you are suggesting to perform a check for every parsed macro definition that would identify whether the macro has a name matching what is provided in -cl-ext=? Then if the name matches it would check whether the macro should be defined or not?

Perhaps if you give me an example it would help to understand

Yes, in general this approach looks good to me conceptually. I have two suggestions:

What did you think in mind regarding activation of features for SPIR?

Addressed all concerns except replacing __opencl_c_int64 definition into header. The reason for this as follows: this macro should be predefined regardless if clang includes default header or not.

I guess targets like SPIR will be supporting all features by default?

Looks good to me, thanks for the note.

Anastasia, Sven, thanks for the review! Anastasia, it seems that I don't have write access yet. Would you mind landing this change on behalf of me? Thanks!

In D88300#2302796, @svenvh wrote:

LGTM! I'd suggest waiting a bit before committing this though, to give people time to catch up on the RFC.