[OpenCL] Add support of OpenCL C 3.0 __opencl_c_fp64

[azabaznov]

Summary

There already exists cl_khr_fp64 extension. So OpenCL C 3.0
and higher should use the feature, earlier versions still
use the extension. OpenCL C 3.0 API spec states that extension
will be not described in the option string if corresponding
optional functionality is not supported (see 4.2. Querying Devices).
Due to that fact the usage of features for OpenCL C 3.0 must
be as follows:

$ clang -Xclang -cl-ext=+cl_khr_fp64,+__opencl_c_fp64 ...

$ clang -Xclang -cl-ext=-cl_khr_fp64,-__opencl_c_fp64 ...

e.g. the feature and the equivalent extension (if exists)
must be set to the same values

[svenvh]

clang/include/clang/Basic/OpenCLOptions.h

157 ↗

(On Diff #323064)

The argument "Ext" suggests "Extension", so perhaps rename if this is about features? (also in the definition in the .cpp file)

clang/lib/Basic/TargetInfo.cpp

395 ↗

(On Diff #323064)

simultaneosly -> simultaneously
correspoding -> corresponding

clang/lib/Headers/opencl-c.h

4635 ↗

(On Diff #323064)

Wondering if we need a similar change in clang/lib/Headers/opencl-c-base.h to guard the double<N> vector types?

[Anastasia]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10027–10032

We shouldn't require enabling the features besides features and extensions are conceptually the same, let's avoid adding too much complexity due to the unfortunate differences in the spec wording.

I suggest changing this to something like:

use of %select{type|declaration}0 %1 requires %2 support

FYI I would also be happy if we don't guard the types by the pragma at all and just allow using the type freely if the extensions is supported. This already happens for the functions so it is not clear why functions and types should be different. The benefit of the pragma guarding hasn't been found after more than a year of investigations.

clang/lib/Basic/OpenCLOptions.cpp

24 ↗

(On Diff #323064)

We should at least be checking for isSupported("__opencl_c_fp64"), but frankly I would prefer to check for supported and not for enabled for cl_khr_fp64 too. Note that we don't break backward compatibility if we change this because the existing kernels will still be valid and it makes things easier for writing new kernels too.

clang/lib/Basic/TargetInfo.cpp

398 ↗

(On Diff #323064)

I suggest we move this onto OpenCLOptions::addSupport.

clang/lib/Headers/opencl-c.h

4635 ↗

(On Diff #323064)

Instead of growing the guard condition, I suggest we only check for one for example the feature macro and then make sure the feature macro is defined in opencl-c-base.h if the extensions that aliases to the feature is supported. However, it would also be ok to do the opposite since the extensions and the corresponding features should both be available.

FYI, something similar was done in https://reviews.llvm.org/D92004.

This will also help to propagate the functionality into Tablegen header because we won't need to extend it to work with multiple extensions but we might still need to do the rename.

clang/lib/Sema/Sema.cpp

356 ↗

(On Diff #323064)

I think we should just guard

addImplicitTypedef("atomic_double", AtomicDoubleT);

by the feature support instead...

Or alternatively, we could also just change this entire diagnostic set up to check for extensions and features being supported and not enabled.

Frankly, I would prefer the first solution because this aligns with C/C++ concept. It is not possible for the compiler to know all possible types and functions that are added via the libraries so this diagnostic only worked for some cases (that are added to the compiler) but not all cases. However, I don't think we need to expose to the developer where and how features are implemented. This will avoid unnessasary questions - why in some cases they get one diagnostic and in the other case they get another diagnostic. It will allow us to change the implementation without the developers to notice. And considering that this doesn't break backward compatibility it is a pretty reasonable direction in my opinion.

clang/test/SemaOpenCL/opencl-fp64-feature.cl

1 ↗

(On Diff #323064)

I suggest we merge this with extensions.cl that has similar functionality. As it mainly tests doubles I don't mind if you prefer to rename it then.

[azabaznov]

clang/include/clang/Basic/OpenCLOptions.h

157 ↗

(On Diff #323064)

Sure, thanks. I was going to rename 'extensions' to other concept which will represent extensions and features, but this will be done in a separate commit

clang/lib/Basic/OpenCLOptions.cpp

24 ↗

(On Diff #323064)

I think everything fine with this for now because OpenCLOptions::enableSupportedCore is called to set all supported core or optional core features to enabled state. So you suggest to removing this method at all too?

I think with your approach things will be unobvious in code: for some extensions there will be check for isSupported for some other there will be check for isEnabled. I think we should stay consistent here and check availability of all options in the same manner.

clang/lib/Basic/TargetInfo.cpp

395 ↗

(On Diff #323064)

Sure, thanks

398 ↗

(On Diff #323064)

This should stay here to control simultaneous macro definitions

clang/lib/Headers/opencl-c.h

4635 ↗

(On Diff #323064)

Yeah, I overlooked this place... Thanks!

4635 ↗

(On Diff #323064)

I don't think that growing of this condition will hurt in some cases... Note, that unifying condition check makes sense if some features are unconditionally supported for OpenCL C 2.0, such as generic address space for example. In other cases (such as fp64, 3d image writes, subgroups) we should use OR in guard condition.

Your approach will require extra checks in clang such as, for example, to make sure that extension macro is not predefined if the feature macro is defined, because there will be redefinition since extension macro is defined in header.

clang/lib/Sema/Sema.cpp

356 ↗

(On Diff #323064)

I think we should just guard

Hmm, I've never though of it but is sounds like a good idea. I'll double-check that and see how it will work!

clang/test/SemaOpenCL/opencl-fp64-feature.cl

1 ↗

(On Diff #323064)

Sure, I'll try that. At first, I just didn't want to modify test's  guard checks as it will become messy:

#if __OPENCL_C_VERSION__ >= 300
// expected-error@-2{{use of type 'double' requires __opencl_c_fp64 feature to be enabled}}
#else
// expected-error@-4{{use of type 'double' requires cl_khr_fp64 feature to be enabled}}
#endif

I see if I can avoid that. Maybe also try to unify the diagnostic for that case?

[Anastasia]

clang/lib/Basic/OpenCLOptions.cpp

24 ↗

(On Diff #323064)

I think everything fine with this for now because OpenCLOptions::enableSupportedCore is called to set all supported core or optional core features to enabled state. So you suggest to removing this method at all too?

Yes, I find it redundant somehow. Maybe it's best to indeed remove enabling functionality for features since we definitely don't plan to use pragmas for those? However, I appreciate it might be better to do as a separate change.

I think with your approach things will be unobvious in code: for some extensions there will be check for isSupported for some other there will be check for isEnabled. I think we should stay consistent here and check availability of all options in the same manner.

That's right, we might get some inconsistency at the start. But I think we should drive towards checking isSupported rather than isEnabled. I don't think we will have many cases for isEnabled at the end.

clang/lib/Basic/TargetInfo.cpp

398 ↗

(On Diff #323064)

Could we leave this bit out? These are set correctly by the targets already... and I think targets do need to set those explicitly indeed. I don't see big value in this functionality right now.

clang/lib/Headers/opencl-c.h

4635 ↗

(On Diff #323064)

I think using one macro for everything is just simpler and cleaner.

Your approach will require extra checks in clang such as, for example, to make sure that extension macro is not predefined if the feature macro is defined, because there will be redefinition since extension macro is defined in header.

I think we should handle this in the headers instead and we should definitely define the macros conditionally to avoid redefinitions.

clang/test/SemaOpenCL/opencl-fp64-feature.cl

1 ↗

(On Diff #323064)

I agree, perhaps regular expressions in the diagnostics could help too i.e. expected-error-re.

[azabaznov]

clang/lib/Basic/OpenCLOptions.cpp

24 ↗

(On Diff #323064)

Thanks for feedback. I did some refactoring needed for this patch to proceed: https://reviews.llvm.org/D97058. AFAIU it doesn't conflict with the one you did (https://reviews.llvm.org/D97052).

[azabaznov]

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

[azabaznov]

clang/lib/Basic/TargetInfo.cpp

398 ↗

(On Diff #323064)

There are 2 reasons why it should be here

1. Simultaneous macro definition

2. Correct option processing: we need to remove cl_khr_fp64 macro if -cl-ext=-__opencl_c_fp64 specified

[azabaznov]

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

[Anastasia]

I would prefer if we try to take a slightly different route i.e. if two features are identical (e.g. cl_khr_fp64 and __opencl_c_fp64) we make sure that they are both set identical in Target options or command-line interface using early check and a diagnostic in the driver. This could be done at the end of the frontend options setup. Then for the rest of the compilation including header parsing, we can confidently keep checking only one of them because we know that they are both set consistently and that they are indeed identical. If you stick to checking for cl_khr_fp64 you can avoid most of the modifications completely.

I have added more details in the code comments. :)

clang/lib/Basic/OpenCLOptions.cpp

41 ↗

(On Diff #336434)

btw since cl_khr_fp64 is available in all versions could we not just check only for it?

clang/lib/Basic/TargetInfo.cpp

398 ↗

(On Diff #323064)

Ok as I said I would prefer to stick to an explicit interface though, i.e. every feature should be set explicitly
-cl-ext=-__opencl_c_fp64,-cl_khr_fp64
as the interface gets too messy otherwise...

I believe we won't have that many of those case - is it just fp64, fp16 and images?

If you are worried about the inconsistent uses (which is a valid concern!) how about we just add a check with a diagnostic in clang driver about the consistency of the setup between the language, command-line options and the target. We already discussed such check in other use cases - to help avoid using incorrect language versions that the target has no support.

I think this route will be easier than keeping consistency automatically and silently overriding the options i.e. we already had bad experiences with bugs due to such logic. A diagnostic would be very helpful both to application developers and tooling developers and help to avoid extra complexity in clang too.

clang/lib/Headers/opencl-c-base.h

530 ↗

(On Diff #336434)

Let's only use one macro as they signal the same. I don't mind which one you choose perhaps __opencl_c_fp64 could be used since we can freely define it in the earliest versions or we can continue using cl_khr_fp64 then no changes are needed other then making sure it is defined when __opencl_c_fp64 is defined which can be done at the top of this header btw.

clang/lib/Headers/opencl-c.h

4635 ↗

(On Diff #323064)

Do you still plan to address this? FYI this comment is the same as in opencl-c-base.h.

clang/lib/Sema/Sema.cpp

305 ↗

(On Diff #336434)

But cl_khr_fp64 is also available if __opencl_c_fp64 is available. If anything it should be even more available for backward compatibility. I don't see the point in checking the same thing twice or checking it differently.

If you are worried you could add some comment/documentation to clarify that they both have to be set or unset simultaneously and a check in the Clang driver initialization verifying the consistency in the settings.

[azabaznov]

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.

[Anastasia]

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?

[azabaznov]

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?

Sure, agree. FYI I'm looking to a proper way of validating a target already. There already exists TargetInfo::validateTarget. I wanted to reuse that (just don't want to introduce yet another interface for OpenCL in Target), but I'm again bumping into some problems because of our special OpenCL options settings: a target should be immutable once created; and language options are not used to create a target. I'm just wondering if there is some more refactoring is needed...

[Anastasia]

In D96524#2705192, @azabaznov wrote:

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?

Sure, agree. FYI I'm looking to a proper way of validating a target already. There already exists TargetInfo::validateTarget. I wanted to reuse that (just don't want to introduce yet another interface for OpenCL in Target), but I'm again bumping into some problems because of our special OpenCL options settings: a target should be immutable once created; and language options are not used to create a target. I'm just wondering if there is some more refactoring is needed...

I see, if you need any help feel free to provide more details and we can discuss it further. :)

[azabaznov]

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

I think unifying with target features requires more effort than it seemed because target features are declared in LLVM (see for example AMDGPUFeatureKV in AMDGPUGenSubtargetInfo.inc, llvm/lib/Target/AMDGPU/AMDGPU.td)

[azabaznov]

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

clGetDeviceInfo: Will not describe support for the cl_khr_3d_image_writes extension if device does not support writing to 3D image objects.

Will update docs.

[Anastasia]

clang/include/clang/Basic/DiagnosticCommonKinds.td

369

We don't normally start from the upper case:

Options -> options

I am thinking we could drop "which is illegal in OpenCL C 3.0" because the fact that compiler gives an error indicates that it is illegal. So I find it a bit redundant.

clang/include/clang/Basic/DiagnosticSemaKinds.td

121–122

I am thinking that in OpenCL 3 both cl_khr_fp64 and __opencl_c_fp64 are required. Maybe we should re-word this as follows?

clang/lib/Sema/Sema.cpp

306 ↗

(On Diff #343417)

Technically both "__opencl_c_fp64" and "cl_khr_fp64" are required in OpenCL, perhaps we should report both in OpenCL 3?

[azabaznov]

clang/lib/Sema/Sema.cpp

306 ↗

(On Diff #343417)

Even better, can we merge this first https://reviews.llvm.org/D100976? This is needed to completely remove such check for fp64.

[Anastasia]

clang/lib/Sema/Sema.cpp

306 ↗

(On Diff #343417)

Sure I have committed it now: https://reviews.llvm.org/rG769cc335e6e63e5eac0c0ac849de44714326e20b

Btw, I will also commit https://reviews.llvm.org/D101043 soon. That should hopefully remove the remianing confusions around pragmas.

[azabaznov]

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

[Anastasia]

LGTM! Thanks!

clang/lib/Sema/Sema.cpp

304 ↗

(On Diff #345846)

I suggest to drop changes in this file since they are formatting only.

clang/test/CodeGenOpenCL/printf.cl

9

I think we don't technically need this change?

[azabaznov]

Drop formatting changes from Sema.cpp

[azabaznov]

Drop one more formatting change from Sema.cpp

[azabaznov]

clang/test/CodeGenOpenCL/printf.cl

9

To be honest I am not sure. I think per OpenCL C spec extension feature macros should be used exactly as follows:

...
if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
...

So I think this is OK to have such checks in tests as soon as we have functionality for simultaneous support (opencl-c-3.0.incorrect_options.cl). If we decide to change that behaviour - this test will fail.

clang/docs/OpenCLSupport.rst

 Missing features or with limited support
 ----------------------------------------
 
 - IR generation for global destructors is incomplete (See:
   `PR48047 <https://llvm.org/PR48047>`_).
 
 .. _opencl_300:
 
-OpenCL 3.0 Implementation Status
+OpenCL C 3.0 Usage
+================================
+
+OpenCL C 3.0 language standard makes most OpenCL C 2.0 features optional. Optional
+functionality in OpenCL C 3.0 is indicated with the presence of feature-test macros
+(list of feature-test macros is `here <https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#features>`_).
+Command-line flag :ref:`-cl-ext <opencl_cl_ext>` can be used to override features supported by a target.
+
+For cases when there is an associated extension for a specific feature (fp64 and 3d image writes)
+user should specify both (extension and feature) in command-line flag:
+
+   .. code-block:: console
+
+     $ clang -cc1 -cl-std=CL3.0 -cl-ext=+cl_khr_fp64,+__opencl_c_fp64 ...
+     $ clang -cc1 -cl-std=CL3.0 -cl-ext=-cl_khr_fp64,-__opencl_c_fp64 ...
+
+
+OpenCL C 3.0 Implementation Status
 ================================
 
 The following table provides an overview of features in OpenCL C 3.0 and their
 implementation status.
 
 +------------------------------+--------------------------------------------------------------+----------------------+---------------------------------------------------------------------------+
 | Category                     | Feature                                                      | Status               | Reviews                                                                   |
 +==============================+==============================================================+======================+===========================================================================+

clang/include/clang/Basic/DiagnosticCommonKinds.td

 // Poison system directories.
 def warn_poison_system_directories : Warning <
   "include location '%0' is unsafe for cross-compilation">,
   InGroup<DiagGroup<"poison-system-directories">>, DefaultIgnore;
 
 def warn_opencl_unsupported_core_feature : Warning<
   "%0 is a core feature in %select{OpenCL C|C++ for OpenCL}1 version %2 but not supported on this target">,
   InGroup<OpenCLCoreFeaturesDiagGroup>, DefaultIgnore;
+
+def err_opencl_extension_and_feature_differs : Error<
+  "options %0 and %1 are set to different values">;
 }

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_predef_outside_function : Warning<
   InGroup<DiagGroup<"predefined-identifier-outside-function">>;
 def warn_float_overflow : Warning<
   "magnitude of floating-point constant too large for type %0; maximum is %1">,
    InGroup<LiteralRange>;
 def warn_float_underflow : Warning<
   "magnitude of floating-point constant too small for type %0; minimum is %1">,
   InGroup<LiteralRange>;
 def warn_double_const_requires_fp64 : Warning<
-  "double precision constant requires cl_khr_fp64, casting to single precision">;
+  "double precision constant requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0, "
+  "casting to single precision">;
 def err_half_const_requires_fp16 : Error<
   "half precision constant requires cl_khr_fp16">;
 
 // C99 variable-length arrays
 def ext_vla : Extension<"variable length arrays are a C99 feature">,
   InGroup<VLAExtension>;
 def warn_vla_used : Warning<"variable length array used">,
   InGroup<VLA>, DefaultIgnore;

 def err_opencl_type_not_found : Error<
   "%0 type %1 not found; include the base header with -finclude-default-header">;
 def warn_opencl_attr_deprecated_ignored : Warning <
   "%0 attribute is deprecated and ignored in OpenCL version %1">,
   InGroup<IgnoredAttributes>;
 def err_opencl_variadic_function : Error<
   "invalid prototype, variadic arguments are not allowed in OpenCL">;
 def err_opencl_requires_extension : Error<
   "use of %select{type|declaration}0 %1 requires %2 support">;
 def ext_opencl_double_without_pragma : Extension<
   "Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is"
   " supported">;
+def err_opencl_double_requires_extension :  Error<
+    "use of type 'double' requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0 support">;
 def warn_opencl_generic_address_space_arg : Warning<
   "passing non-generic address space pointer to %0"
   " may cause dynamic conversion affecting performance">,
   InGroup<Conversion>, DefaultIgnore;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
 def err_opencl_builtin_pipe_first_arg : Error<
   "first argument to %0 must be a pipe type">;

clang/lib/Basic/Targets.cpp

if (OpenCLOptions::isOpenCLOptionCoreIn(Opts, OptArgs...) &&
       Diags.Report(diag::warn_opencl_unsupported_core_feature)
           << Name << Opts.OpenCLCPlusPlus
           << Opts.getOpenCLVersionTuple().getAsString();
   };
 #define OPENCL_GENERIC_EXTENSION(Ext, ...)                                     \
   diagnoseNotSupportedCore(#Ext, __VA_ARGS__);
 #include "clang/Basic/OpenCLExtensions.def"
 
-  // For now assume that OpenCL target is always
-  // valid and just provide necessary diagnostics
+  // Validate that feature macros are set properly for OpenCL C 3.0.
+  // In other cases assume that target is always valid.
+  if (Opts.OpenCLCPlusPlus || Opts.OpenCLVersion < 300)
+    return true;
+
+  // Feature and corresponding equivalent extension must be set
+  // simultaneously to the same value.
+  for (auto &ExtAndFeat : {std::make_pair("cl_khr_fp64", "__opencl_c_fp64")})
+    if (hasFeatureEnabled(OpenCLFeaturesMap, ExtAndFeat.first) !=
+        hasFeatureEnabled(OpenCLFeaturesMap, ExtAndFeat.second)) {
+      Diags.Report(diag::err_opencl_extension_and_feature_differs)
+          << ExtAndFeat.first << ExtAndFeat.second;
+      return false;
+    }
+
   return true;
 }

clang/lib/Basic/Targets/AMDGPU.h

void setSupportedOpenCLOpts() override {
     Opts["cl_clang_storage_class_specifiers"] = true;
     Opts["__cl_clang_variadic_functions"] = true;
     Opts["__cl_clang_function_pointers"] = true;
     Opts["__cl_clang_non_portable_kernel_param_types"] = true;
 
     bool IsAMDGCN = isAMDGCN(getTriple());
 
     Opts["cl_khr_fp64"] = hasFP64();
+    Opts["__opencl_c_fp64"] = hasFP64();
 
     if (IsAMDGCN || GPUKind >= llvm::AMDGPU::GK_CEDAR) {
       Opts["cl_khr_byte_addressable_store"] = true;
       Opts["cl_khr_global_int32_base_atomics"] = true;
       Opts["cl_khr_global_int32_extended_atomics"] = true;
       Opts["cl_khr_local_int32_base_atomics"] = true;
       Opts["cl_khr_local_int32_extended_atomics"] = true;
     }

clang/lib/Basic/Targets/NVPTX.h

public:
   void setSupportedOpenCLOpts() override {
     auto &Opts = getSupportedOpenCLOpts();
     Opts["cl_clang_storage_class_specifiers"] = true;
     Opts["__cl_clang_function_pointers"] = true;
     Opts["__cl_clang_variadic_functions"] = true;
     Opts["__cl_clang_non_portable_kernel_param_types"] = true;
 
     Opts["cl_khr_fp64"] = true;
+    Opts["__opencl_c_fp64"] = true;
     Opts["cl_khr_byte_addressable_store"] = true;
     Opts["cl_khr_global_int32_base_atomics"] = true;
     Opts["cl_khr_global_int32_extended_atomics"] = true;
     Opts["cl_khr_local_int32_base_atomics"] = true;
     Opts["cl_khr_local_int32_extended_atomics"] = true;
   }
 
   /// \returns If a target requires an address within a target specific address

clang/lib/Frontend/CompilerInstance.cpp

if (getLangOpts().getFPExceptionMode() != LangOptions::FPE_Ignore) {
       getDiagnostics().Report(diag::warn_fe_backend_unsupported_fp_exceptions);
       getLangOpts().setFPExceptionMode(LangOptions::FPE_Ignore);
     }
     // FIXME: can we disable FEnvAccess?
   }
 
   // We should do it here because target knows nothing about
   // language options when it's being created.
-  if (getLangOpts().OpenCL)
-    getTarget().validateOpenCLTarget(getLangOpts(), getDiagnostics());
+  if (getLangOpts().OpenCL &&
+      !getTarget().validateOpenCLTarget(getLangOpts(), getDiagnostics()))
+    return false;
 
   // Inform the target of the language options.
   // FIXME: We shouldn't need to do this, the target should be immutable once
   // created. This complexity should be lifted elsewhere.
   getTarget().adjust(getLangOpts());
 
   // Adjust target options based on codegen options.
   getTarget().adjustTargetOptions(getCodeGenOpts(), getTargetOpts());

clang/lib/Sema/SemaExpr.cpp

if (Literal.isFixedPointLiteral()) {
     if (Ty == Context.DoubleTy) {
       if (getLangOpts().SinglePrecisionConstants) {
         if (Ty->castAs<BuiltinType>()->getKind() != BuiltinType::Float) {
           Res = ImpCastExprToType(Res, Context.FloatTy, CK_FloatingCast).get();
         }
       } else if (getLangOpts().OpenCL && !getOpenCLOptions().isAvailableOption(
                                              "cl_khr_fp64", getLangOpts())) {
         // Impose single-precision float type when cl_khr_fp64 is not enabled.
-        Diag(Tok.getLocation(), diag::warn_double_const_requires_fp64);
+        Diag(Tok.getLocation(), diag::warn_double_const_requires_fp64)
+            << (getLangOpts().OpenCLVersion >= 300);
         Res = ImpCastExprToType(Res, Context.FloatTy, CK_FloatingCast).get();
       }
     }
   } else if (!Literal.isIntegerLiteral()) {
     return ExprError();
   } else {
     QualType Ty;
 

clang/lib/Sema/SemaType.cpp

if (!S.Context.getTargetInfo().hasBFloat16Type())
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)
         << "__bf16";
     Result = Context.BFloat16Ty;
     break;
   case DeclSpec::TST_float:   Result = Context.FloatTy; break;
   case DeclSpec::TST_double:
     if (S.getLangOpts().OpenCL) {
       if (!S.getOpenCLOptions().isSupported("cl_khr_fp64", S.getLangOpts()))
-        S.Diag(DS.getTypeSpecTypeLoc(), diag::err_opencl_requires_extension)
-          << 0 << Context.DoubleTy << "cl_khr_fp64";
+        S.Diag(DS.getTypeSpecTypeLoc(),
+               diag::err_opencl_double_requires_extension)
+            << (S.getLangOpts().OpenCLVersion >= 300);
       else if (!S.getOpenCLOptions().isAvailableOption("cl_khr_fp64", S.getLangOpts()))
         S.Diag(DS.getTypeSpecTypeLoc(), diag::ext_opencl_double_without_pragma);
     }
     if (DS.getTypeSpecWidth() == TypeSpecifierWidth::Long)
       Result = Context.LongDoubleTy;
     else
       Result = Context.DoubleTy;
     break;

clang/test/CodeGenOpenCL/printf.cl

 // RUN: %clang_cc1 -cl-std=CL1.2 -cl-ext=-+cl_khr_fp64 -triple spir-unknown-unknown -disable-llvm-passes -emit-llvm -o - %s | FileCheck -check-prefixes=FP64,ALL %s
 // RUN: %clang_cc1 -cl-std=CL1.2 -cl-ext=-cl_khr_fp64 -triple spir-unknown-unknown -disable-llvm-passes -emit-llvm -o - %s | FileCheck -check-prefixes=NOFP64,ALL %s
+// RUN: %clang_cc1 -cl-std=CL3.0 -cl-ext=+__opencl_c_fp64,+cl_khr_fp64 -triple spir-unknown-unknown -disable-llvm-passes -emit-llvm -o - %s | FileCheck -check-prefixes=FP64,ALL %s
+// RUN: %clang_cc1 -cl-std=CL3.0 -cl-ext=-__opencl_c_fp64,-cl_khr_fp64 -triple spir-unknown-unknown -disable-llvm-passes -emit-llvm -o - %s | FileCheck -check-prefixes=NOFP64,ALL %s
 
 typedef __attribute__((ext_vector_type(2))) float float2;
 typedef __attribute__((ext_vector_type(2))) half half2;
 
-#ifdef cl_khr_fp64
+#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
 typedef __attribute__((ext_vector_type(2))) double double2;
 #endif
 
 int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 2)));
 
 
 // ALL-LABEL: @test_printf_float2(
 // FP64: %call = call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 addrspace(2)* getelementptr inbounds ([7 x i8], [7 x i8] addrspace(2)* @.str, i32 0, i32 0), <2 x float> %0)
 
 
 // NOFP64:  call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 addrspace(2)* getelementptr inbounds ([7 x i8], [7 x i8] addrspace(2)* @.str, i32 0, i32 0), <2 x float> %0)
 kernel void test_printf_float2(float2 arg) {
   printf("%v2hlf", arg);
 }
 
 // ALL-LABEL: @test_printf_half2(
 // FP64:  %call = call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 addrspace(2)* getelementptr inbounds ([6 x i8], [6 x i8] addrspace(2)* @.str.1, i32 0, i32 0), <2 x half> %0)
 
 // NOFP64:  %call = call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 addrspace(2)* getelementptr inbounds ([6 x i8], [6 x i8] addrspace(2)* @.str.1, i32 0, i32 0), <2 x half> %0)
 kernel void test_printf_half2(half2 arg) {
   printf("%v2hf", arg);
 }
 
-#ifdef cl_khr_fp64
+#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
 // FP64-LABEL: @test_printf_double2(
 // FP64: call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 addrspace(2)* getelementptr inbounds ([6 x i8], [6 x i8] addrspace(2)* @.str.2, i32 0, i32 0), <2 x double> %0)
 kernel void test_printf_double2(double2 arg) {
   printf("%v2lf", arg);
 }
 #endif

clang/test/Misc/opencl-c-3.0.incorrect_options.cl

+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=-__opencl_c_fp64,+cl_khr_fp64 %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=+__opencl_c_fp64,-cl_khr_fp64 %s 2>&1 | FileCheck %s
+
+// CHECK: error: options cl_khr_fp64 and __opencl_c_fp64 are set to different values

clang/test/SemaOpenCL/fp64-fp16-options.cl

 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.1
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL1.1 -DNOPEDANTIC
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.2 -DFP64
 
 // Test with a target not supporting fp64.
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -DNOFP64 -DNOFP16
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -DNOFP64 -DNOFP16
 
 // Test with some extensions enabled or disabled by cmd-line args
 //
 // Target does not support fp64 and fp16 - override it
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+cl_khr_fp64,+cl_khr_fp16
 //
 // Disable or enable all extensions
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -DNOFP64 -DNOFP16
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all,-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=-all,+cl_khr_fp64 -DNOFP16
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -DNOFP64 -DNOFP16
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all -DFP64
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all,-__opencl_c_fp64,-cl_khr_fp64 -DNOFP64
 //
 // Concatenating
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-cl_khr_fp64,+cl_khr_fp64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64 -cl-ext=+cl_khr_fp16 -cl-ext=-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64,-cl_khr_fp64,+cl_khr_fp16 -DNOFP64
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-__opencl_c_fp64,-cl_khr_fp64 -cl-ext=+__opencl_c_fp64,+cl_khr_fp64 -DFP64
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-__opencl_c_fp64,+__opencl_c_fp64,+cl_khr_fp64 -DFP64
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-__opencl_c_fp64,-cl_khr_fp64 -DNOFP64
 
 // Test with -finclude-default-header, which includes opencl-c.h. opencl-c.h
 // disables all extensions by default, but supported core extensions for a
 // particular OpenCL version must be re-enabled (for example, cl_khr_fp64 is
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=clc++

 #pragma OPENCL EXTENSION cl_khr_fp16 : enable
 #ifdef NOFP16
 // expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp16' - ignoring}}
 #endif
 
 void f2(void) {
   double d;
 #ifdef NOFP64
-// expected-error@-2{{use of type 'double' requires cl_khr_fp64 support}}
+#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ >= 300)
+// expected-error@-3{{use of type 'double' requires cl_khr_fp64 and __opencl_c_fp64 support}}
+#else
+// expected-error@-5{{use of type 'double' requires cl_khr_fp64 support}}
+#endif
 #endif
 
   typedef double double4 __attribute__((ext_vector_type(4)));
   double4 d4 = {0.0f, 2.0f, 3.0f, 1.0f};
 #ifdef NOFP64
-// expected-error@-3 {{use of type 'double' requires cl_khr_fp64 support}}
+#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ >= 300)
+// expected-error@-4 {{use of type 'double' requires cl_khr_fp64 and __opencl_c_fp64 support}}
+#else
+// expected-error@-6 {{use of type 'double' requires cl_khr_fp64 support}}
+#endif
 #endif
 
   (void) 1.0;
 #ifdef NOFP64
-// expected-warning@-2{{double precision constant requires cl_khr_fp64, casting to single precision}}
+#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ >= 300)
+// expected-warning@-3{{double precision constant requires cl_khr_fp64 and __opencl_c_fp64, casting to single precision}}
+#else
+// expected-warning@-5{{double precision constant requires cl_khr_fp64, casting to single precision}}
+#endif
 #endif
 }
 
 #pragma OPENCL EXTENSION cl_khr_fp64 : disable
 #ifdef NOFP64
 // expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp64' - ignoring}}
 #endif