This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add support of OpenCL C 3.0 __opencl_c_fp64
ClosedPublic

Authored by azabaznov on Feb 11 2021, 9:57 AM.

Details

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

Diff Detail

Event Timeline

azabaznov created this revision.Feb 11 2021, 9:57 AM
azabaznov requested review of this revision.Feb 11 2021, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 9:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
svenvh added inline comments.Feb 12 2021, 3:41 AM
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 added inline comments.Feb 12 2021, 8:52 AM
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 added inline comments.Feb 15 2021, 1:18 AM
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 added inline comments.Feb 15 2021, 5:35 AM
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 marked 3 inline comments as done.Feb 20 2021, 12:15 AM
azabaznov added inline comments.
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 updated this revision to Diff 336430.Apr 9 2021, 6:16 AM

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

azabaznov added inline comments.Apr 9 2021, 6:19 AM
clang/lib/Basic/TargetInfo.cpp
398 ↗(On Diff #323064)

There are 2 reasons why it should be here

  1. Simultaneous macro definition
  1. Correct option processing: we need to remove cl_khr_fp64 macro if -cl-ext=-__opencl_c_fp64 specified
azabaznov updated this revision to Diff 336434.Apr 9 2021, 6:28 AM

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

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.

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.

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?

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

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

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 updated this revision to Diff 343417.May 6 2021, 8:23 AM

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.

azabaznov edited the summary of this revision. (Show Details)May 6 2021, 8:25 AM
azabaznov edited the summary of this revision. (Show Details)May 6 2021, 8:28 AM
Anastasia added inline comments.May 10 2021, 7:56 AM
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 added inline comments.May 14 2021, 7:43 AM
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 added inline comments.May 14 2021, 10:07 AM
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 updated this revision to Diff 345846.May 17 2021, 5:42 AM
azabaznov edited the summary of this revision. (Show Details)

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

azabaznov marked 4 inline comments as done.May 17 2021, 5:43 AM
Anastasia accepted this revision.May 18 2021, 2:25 AM

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?

This revision is now accepted and ready to land.May 18 2021, 2:25 AM
azabaznov updated this revision to Diff 346379.May 19 2021, 3:08 AM

Drop formatting changes from Sema.cpp

azabaznov updated this revision to Diff 346380.May 19 2021, 3:11 AM

Drop one more formatting change from Sema.cpp

azabaznov marked an inline comment as done.May 19 2021, 3:18 AM
azabaznov added inline comments.
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.

This revision was landed with ongoing or failed builds.May 21 2021, 5:01 AM
This revision was automatically updated to reflect the committed changes.