Page MenuHomePhabricator

[OpenCL] Add support of OpenCL C 3.0 __opencl_c_fp64
Needs ReviewPublic

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

Details

Reviewers
Anastasia
svenvh
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.

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
169

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
398

simultaneosly -> simultaneously
correspoding -> corresponding

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

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
9948

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
41

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
401

I suggest we move this onto OpenCLOptions::addSupport.

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

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
369

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
169

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
41

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
398

Sure, thanks

401

This should stay here to control simultaneous macro definitions

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

Yeah, I overlooked this place... Thanks!

4635

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
369

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
41

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
401

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

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
41

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.Fri, Apr 9, 6:16 AM

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

azabaznov added inline comments.Fri, Apr 9, 6:19 AM
clang/lib/Basic/TargetInfo.cpp
401

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.Fri, Apr 9, 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

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

clang/lib/Basic/TargetInfo.cpp
401

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

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

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

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)