In D97869#2807654, @svenvh wrote:

In D97869#2807627, @azabaznov wrote:

@svenvh, are you going to add header-like output emitting?

Yes, I can add that if there is interest. I already have it on a local branch, so if there is interest (and it seems there is? :-)), then I will rebase that, tidy it up, and create a separate review.

@svenvh, are you going to add header-like output emitting?

In D102853#2773890, @Anastasia wrote:

LGTM! Although I think it would be better if we set __IMAGE_SUPPORT__ conditionally also for earlier OpenCL versions.

Drop one more formatting change from Sema.cpp

Drop formatting changes from Sema.cpp

Rebase after https://reviews.llvm.org/D100976; addressed review comments and updated docs.

Rebased to use ::validateOpenCLTarget. Decided to use explicit simulatneous extensions/feature disabling in command line due to API spec limitations:

Thanks! Looks good to me in general. See a comment.

LGTM. Thanks!

Not sure what do we want to achieve with this? Do you want to point out that the code might be somehow less portable let's say between clang revisions, etc?

Add test for C++ for OpenCL diagnostics

In D100980#2719322, @Anastasia wrote:

In D100980#2719196, @azabaznov wrote:

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

I mean diagnose once when, for example in your particular case, double type is parsed. Does it require much effort? I think this warning might be useful for developers who already rely on pragma usage in their kernels.

I am not sure I understand your suggestion. Could you show an example perhaps?

Generally looks good to me, but maybe a test needed, see a comment. Thanks! But I'm still not sure about completely removing pragmas for type declarations (see https://reviews.llvm.org/D100980#2719196).

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

I am not sure, to be honest I personally think the extension pragma is a spec failure as it is not specified properly or to allow reasonable implementation

We could of course keep it just for this particular case of doubles, but even half is allowed in certain circumstances without the pragma and it is still an extension. https://godbolt.org/z/K34sP81nx

Use LangOptions::getOpenCLVersionTuple() to provide diagnostics of OpenCL version

For cl_khr_fp64 I still want to keep the pragma for the other use case - to convert double literal into single-precision (https://github.com/KhronosGroup/OpenCL-Docs/issues/578). The reason why I think it could be useful is that the precision change might lead to a different result depending on the precision of the calculation. So I think pragmas could be useful to control whether double literal is single-precision or not to avoid this problem occur when code is compiled for different targets?

Addressed comments, did some more refactoring. Is it OK to have "2.0" diagnostics for C++ for OpenCL?

do you think it is valuable to keep this behavior at all?

LGTM!

Please see https://reviews.llvm.org/D101087.

Btw I am not suggesting removing the pragma. We will still have to parse it for backward compatibility. I am only dropping the requirement of using it in order to call get_kernel_max_sub_group_size_for_ndrange or get_kernel_sub_group_count_for_ndrange when the extension is supported. Because I don't see why this would be needed especially that all other functions from subgroups can be called without the pragma https://godbolt.org/z/MYavchPT3.

Same as for https://reviews.llvm.org/D100984, cl_khr_fp64 wasn't always core and thus it requires pragma for OpenCL C < 1.2 versions.

I wish it could be true... But cl_khr_subgroups still requires pragma in some versions as it wasn't a core feature earlier (https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_subgroups.html).

In D96524#2692376, @Anastasia wrote:

In D96524#2691428, @azabaznov wrote:

My main idea was to provide an interface which will not make users to specify -cl-ext=+__opencl_c_fp64,+cl_khr_fp64/ -cl-ext=-__opencl_c_fp64,-cl_khr_fp64 if they need to enable/disable functionality in OpenCL C 3.0 because I believe that is not a right thing to do: why anyone should care about those extensions if there are features already? Also, this may lead to confusions since, for example, __opencl_c_subgroups and cl_khr_subgroups are not the same (subgroup independent forward progress is required in extension while it is optional in OpenCL C 3.0, thus implementation may support the extension but not the feature).

To be clear: I'm OK with providing a validation of correct option settings (__opencl_c_fp64/cl_khr_fp64 and __opencl_c_3d_image_writes/cl_khr_3d_image_writes should both be set to the same value). Also, it makes sense to unify a check within header and clang to the only macro, I'm OK with that too. But I would prefer to keep the option interface without redundant mentioning of extension.

Ok, let's implement both - we can add an early check of consistency of options and therefore will only need to use one for checking during the parsing and let's simplify the interface of -cl-ext. Since it is a frontend option and new functionality doesn't cause backward compatibility issues i.e. only affects OpenCL 3.0 onwards, so I see no risk. Could you please update the help documentation of the option explaining the new behavior? Then let's concentrate the whole logic for setting the options and keeping the consistency between the equivalent ones in TargetInfo::setCommandLineOpenCLOpts directly? This will provide cleaner flow due to encapsulation and will make it clear where the use case comes from. Does it make sense?

Great! And thanks for fixing misprint :)

My main idea was to provide an interface which will not make users to specify -cl-ext=+__opencl_c_fp64,+cl_khr_fp64/ -cl-ext=-__opencl_c_fp64,-cl_khr_fp64 if they need to enable/disable functionality in OpenCL C 3.0 because I believe that is not a right thing to do: why anyone should care about those extensions if there are features already? Also, this may lead to confusions since, for example, __opencl_c_subgroups and cl_khr_subgroups are not the same (subgroup independent forward progress is required in extension while it is optional in OpenCL C 3.0, thus implementation may support the extension but not the feature).

Remove unrelated change, fixed error in test to check only cl_khr_fp64 for OpenCL C < 1.2

Addressed comments, rebased after refactoring: https://reviews.llvm.org/D97058

Thanks for working on this! Looks good to me in general. I have a small comment.

Thanks for feedback!

Looks good to me. Thanks!

Thanks for  the patch! Sorry for the delay

I have one more though.

@Anastasia, thanks for landing this! Sorry for delay, I though that this can wait until Monday

Do you also get fatal error: 'opencl-c-base.h' file not found? If so, we might need at least to file a clang bug that we should look at before the next release.

Yes, I was able to reproduce the failure with -target arm64-apple-macosx flag, I provided the fix to set explicit target: https://reviews.llvm.org/D98539.

Thanks guys,

Yeah,  I'm looking into it, but I can't reproduce it locally: the test passes at x86_64 linux system. I'll revert the change if it takes too much time to investigate what's going on. Thanks.

Ok, addressing in a separate patch is reasonable, but why do you think that we will break backward compatibility? My current worry is that the implementation is so messy and inconsistent that it will take us longer time if we do the incremental steps. Also, we risk introducing the regressions since it is so hard to make sense out of it.

Replaced atomic_double implicit definition

Corrected some mistakes, added a test for diagnosing undeclared identifiers when a extension is unsupported. Generally leaving the change as it is as completely removing pragma may break backward compatibility now: let's do it in a separate patch and notify everyone before doing that. Also, we should wait before spec will be finalized.

That's awesome!

Check 'isEnabled' is now private: it is used only for non-core or non-optional core features;
creation of implicit type definitions is guarder with extension support check; minor refactoring

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.