Page MenuHomePhabricator

[OpenCL] add CL 3.0 optional feature support to opencl-c.h
Needs ReviewPublic

Authored by airlied on Nov 23 2020, 9:44 PM.

Details

Reviewers
Anastasia
Summary

The opencl-c.h needs a lot of rework to support OpenCL C 3.0 with the optional feature set.

This patch refactors all the C11 atomics, along with changing when generic pointer interfaces can be used and a few other bits.

It will need some decent review and maybe some testing by other CL users to make sure I've haven't messed up when used with a fully featured CL 3.0 stack.

Diff Detail

Event Timeline

airlied created this revision.Nov 23 2020, 9:44 PM
airlied requested review of this revision.Nov 23 2020, 9:44 PM

this file has never been clang-format clean, happy to make changes if reviewer decides they are needed.

Btw how about making some checks simpler. We could always define feature macros __opencl_c_atomic_scope_device, __opencl_c_generic_address_space for OpenCL 2.0 or C++ for OpenCL . Then everywhere we would only need to check feature macros instead of language versions and feature macros together.

#if !(defined()) && (defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0))
#define __opencl_c_atomic_scope_device      1
#define __opencl_c_generic_address_space   1
...
#endif

I assume your change depends on some other changes that add the feature macro definitions. So I think we should link the other changes in at some point. CCing to @azabaznov here who is working on the features too.

this file has never been clang-format clean, happy to make changes if reviewer decides they are needed.

This has not been considered a conventional clang source so to say, so the coding style has not been followed. But it is quite annoying now with formatting hooks being installed. Ideally, it would be good to format this properly but this makes retrieving the history harder and it is extremely valuable. So I suggest not to reformat the old code but for the new code I leave it up to you.

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

I'm being working already at a proper feature macros definition to unify both clang and header OpenCL C 3.0 related changes, here is the change: https://reviews.llvm.org/D89869. So as this patch will be merged then the check for supported feature can be done uniformly in all OpenCL versions:

Use:

#ifdef __opencl_c_pipes

instead of

#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0 && __OPENCL_C_VERSION__ < CL_VERSION_3_0) || (__OPENCL_C_VERSION__ >= CL_VERSION_3_0 && defined(__opencl_c_pipes))

The main concepts are described in RFC thread here: https://lists.llvm.org/pipermail/cfe-dev/2020-September/066883.html. Will be glad to discuss any technical details and concerns. Thanks!

It will need some decent review and maybe some testing by other CL users to make sure I've haven't messed up when used with a fully featured CL 3.0 stack.

Clang functionality is expected to be tested in llvm-project and with OpenCL headers we are violating the process to be honest. Aside from that it slows down the development in my opinion because making larger changes is extremely hard without the testing. Relying on reviews is not great for various reasons. We constantly fix bugs related to the headers.

The biggest issue with testing that header originally was that the parsing of it is so slow - with only one test we have exceeded the acceptable limits immediately. However, to mitigate this problem there is no reason we couldn't add header testing conditioned on a CMake option passed during the build configuration. So by default only minimal functionality could be tested as it is done now but if some option is being passed to cmake (say ENABLE_FULL_OPENCL_BIFS_TESTS) we could do comprehensive testing of the header. If there is interest in such an approach we could start from just adding the tests related to OpenCL 3.0 changes and then slowly build more tests in the future.

Another area where such tests would be needed is for Tablegen header enabled by -fdeclare-opencl-builtins that is developed to replace the regular header eventually because it is much quicker to parse. @svenvh I imagine the testing functionality between the two headers will be identical?

@svenvh I imagine the testing functionality between the two headers will be identical?

Mostly, assuming you intend to test in the conventional way of feeding a .cl file to Clang. Although currently -fdeclare-opencl-builtins only provides a subset of the builtins in opencl-c.h.

In the past I did have a look at the possibility of automated equivalence checking of declarations provided by both -fdeclare-opencl-builtins and opencl-c.h. It shouldn't be impossible, but it's not trivial (one reason being that gentype-expansion is currently done in Sema).

Btw how about making some checks simpler. We could always define feature macros __opencl_c_atomic_scope_device, __opencl_c_generic_address_space for OpenCL 2.0 or C++ for OpenCL . Then everywhere we would only need to check feature macros instead of language versions and feature macros together.

#if !(defined()) && (defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0))
#define __opencl_c_atomic_scope_device      1
#define __opencl_c_generic_address_space   1
...
#endif

I like this idea but my only worry was about leaking those definitions to the user source when the system hasn't defined them.

i.e. a CL2.0 user now sees __opencl_c_generic_address_space, writes code to use that macro, but it fails on other OpenCL C implementations.

But if we are going to have feature code that works like that then I'm fine with going ahead and changing things to look like this.

Btw how about making some checks simpler. We could always define feature macros __opencl_c_atomic_scope_device, __opencl_c_generic_address_space for OpenCL 2.0 or C++ for OpenCL . Then everywhere we would only need to check feature macros instead of language versions and feature macros together.

#if !(defined()) && (defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0))
#define __opencl_c_atomic_scope_device      1
#define __opencl_c_generic_address_space   1
...
#endif

I like this idea but my only worry was about leaking those definitions to the user source when the system hasn't defined them.

i.e. a CL2.0 user now sees __opencl_c_generic_address_space, writes code to use that macro, but it fails on other OpenCL C implementations.

But if we are going to have feature code that works like that then I'm fine with going ahead and changing things to look like this.

Identifiers that start with a double underscore are reserved so there is no real harm in defining them for earlier OpenCL C versions. FYI there is this PR https://github.com/KhronosGroup/OpenCL-Docs/pull/477 where we intend to clarify that the earlier OpenCL C version might have the macro defined too.

airlied updated this revision to Diff 307951.Nov 26 2020, 7:16 PM

I've moved a bunch of longer checks into just feature checks and defined the features at the top of the file.

There are some APIs that are CL 3.0 feature only so those retain the full check.

Thanks! This looks much cleaner.

I am still unclear what should we do with testing though.

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

I feel __OPENCL_C_VERSION__ >= CL_VERSION_3_0 is redundant. Perhaps we don't need __OPENCL_C_VERSION__ >= CL_VERSION_2_0 either if we define the feature macros above. Btw @azabaznov is planning to define those macros in this header in https://reviews.llvm.org/D89869#change-kc4kXsHko6uZ. I guess some rebase will be necessary at some point depending on which commit will be ready first.

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

I guess here it should be:

defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)

There are no OpenCL C language versions between 2.0 and 3.0.

I am still unclear what should we do with testing though.

We probably don't want to hold up this patch until we have more thorough testing in place, since we don't even have any concrete plans for such testing at the moment. Perhaps we should just land the patch once it's ready and if any problems are reported then we can always temporarily revert?

One idea for getting some confidence of not breaking OpenCL 2.0 too much, is to remove the -fdeclare-opencl-builtins flags from the SPIR-V LLVM translator test suite and then run those tests to exercise opencl-c.h a bit.

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

I also think it makes more sense to define the feature macros in opencl-c-base.h, unless there is a reason that I missed?

I am still unclear what should we do with testing though.

We probably don't want to hold up this patch until we have more thorough testing in place, since we don't even have any concrete plans for such testing at the moment.

Well accepting such a significant change that implements the entire functionality of a new standard without a single test doesn't sound a workable approach. We should at least add some amount of testing in existing lib/Headers/opencl-c.h. However, instead of fighting with ad-hoc testing, it might save us time if we agree on a strategy and just do the testing properly. After all it is relatively clear what is to be tested. The issue is mainly with the testing time and amount of overloads to be tested. It should not be too difficult to guard such testing by a cmake option to avoid long execution time of default testing target of clang. However, it might be time-consuming to list all overloads. If we could use C++ templates it would help.

Perhaps we should just land the patch once it's ready and if any problems are reported then we can always temporarily revert?

I am not sure what do you mean by revert temporarily. What if other commits lie on top or depend on this change. Not to say that other repositories might start building functionality on top e.g. SPIRV-LLVM Translator or clspv. I can imagine how this could become a burden for the community. From my experience reverting only works if done within a few days after the commit, then it becomes much harder if not impossible. Another aspect we should consider - how it can impact releasing Clang. What if someone discovers the bug in this header when the release is in preparation? We won't even be able to test the fix adequately especially within releasing time bounds. Or even worst what if a release is out with that bug? Will it be expected from us that a new release will be created with a fix?

It becomes evident that this is no longer experimental functionality and therefore saving on testing will sooner or later lead to a maintenance nightmare. I am not saying that this patch introduced the problems but it just highlights that functionality here is being used and released in products and not in the experimental phase anymore.

One idea for getting some confidence of not breaking OpenCL 2.0 too much, is to remove the -fdeclare-opencl-builtins flags from the SPIR-V LLVM translator test suite and then run those tests to exercise opencl-c.h a bit.

Ok, I think it is generally acceptable. LLVM testing is already very heterogeneous. Would it mean we can test clang commits with such test regularly or would it be done once on this patch only? I anticipate it is likely we will have follow up fix ups.

We probably don't want to hold up this patch until we have more thorough testing in place, since we don't even have any concrete plans for such testing at the moment.

Well accepting such a significant change that implements the entire functionality of a new standard without a single test doesn't sound a workable approach. We should at least add some amount of testing in existing lib/Headers/opencl-c.h. However, instead of fighting with ad-hoc testing, it might save us time if we agree on a strategy and just do the testing properly. After all it is relatively clear what is to be tested. The issue is mainly with the testing time and amount of overloads to be tested. It should not be too difficult to guard such testing by a cmake option to avoid long execution time of default testing target of clang. However, it might be time-consuming to list all overloads. If we could use C++ templates it would help.

Perhaps my assumption that we do not want to hold up this patch until we have more thorough testing in place was wrong then. I agree it would be good to have better header testing of the header. I merely wanted to point out that testing the header thoroughly is a substantial piece of work. So if we want to have such testing in place first, we need to delay this patch and start a thread outside of this review.

One idea for getting some confidence of not breaking OpenCL 2.0 too much, is to remove the -fdeclare-opencl-builtins flags from the SPIR-V LLVM translator test suite and then run those tests to exercise opencl-c.h a bit.

Ok, I think it is generally acceptable. LLVM testing is already very heterogeneous. Would it mean we can test clang commits with such test regularly or would it be done once on this patch only?

This idea would be a one-off test only, done locally. The aim is to increase confidence in the patch, and only that. This idea does not describe any form of regular testing, nor anything close to cover the entire header. I don't see a trivial way of setting this up for regular testing, as the various translator tests should keep the -fdeclare-opencl-builtins flag for speed.

We probably don't want to hold up this patch until we have more thorough testing in place, since we don't even have any concrete plans for such testing at the moment.

Well accepting such a significant change that implements the entire functionality of a new standard without a single test doesn't sound a workable approach. We should at least add some amount of testing in existing lib/Headers/opencl-c.h. However, instead of fighting with ad-hoc testing, it might save us time if we agree on a strategy and just do the testing properly. After all it is relatively clear what is to be tested. The issue is mainly with the testing time and amount of overloads to be tested. It should not be too difficult to guard such testing by a cmake option to avoid long execution time of default testing target of clang. However, it might be time-consuming to list all overloads. If we could use C++ templates it would help.

Perhaps my assumption that we do not want to hold up this patch until we have more thorough testing in place was wrong then. I agree it would be good to have better header testing of the header. I merely wanted to point out that testing the header thoroughly is a substantial piece of work. So if we want to have such testing in place first, we need to delay this patch and start a thread outside of this review.

Yes, this is a substantial amount of work indeed and yes a separate discussion would make sense. One way to approach this could be to start by testing the functionality affected by this patch only. Then we wouldn't necessarily need to hold off until we have the full testing which would take a while. We could split the features being added here into separate patches with tests being added accordingly to each of them. I also think it will simplify the reviewing process and allow the key functionality to be added earlier instead of waiting for everything to be ready at once.

jprice added a subscriber: jprice.Jan 17 2021, 9:25 AM

FYI the testing of headers functionality is now being discussed in https://reviews.llvm.org/D97869

kpet added a subscriber: kpet.Mar 26 2021, 9:04 AM

We have discussed testing of headers within OpenCL developer community and wider:
https://lists.llvm.org/pipermail/cfe-dev/2021-April/068040.html

and the conclusions I have drawn are as follows:

  • There seem to be no big interest in improving upstream testing of opencl-c.h and therefore improving the quality assurance processes in upstream development.
  • There is no plan for opencl-c.h to be exposed to the end user in upstream. An alternative newer solution using Tablegen is currently enabled by default through the clang driver as a primary header.
  • It is likely that opencl-c.h will be deprecated or even removed in the future releases however this discussion hasn't fully taken place yet.

Considering the above and the fact that the opencl-c.h header will continue to exist for the time being as it has been adopted by various out-of-tree implementations of OpenCL it would be reasonable to continue with this patch as it extends in a straightforward way existing functionality with the same experimental quality.

I would quite like to find one extra reviewer from a different vendor preferably with OpenCL 3 expertise. @azabaznov would you be happy to review this?

I would also like to recommend using an experimental test (https://reviews.llvm.org/D97869) that is planned for the Tablegen header solution before committing this patch to gain more confidence. However, It might be good to check with @svenvh first to see whether there are any know issues for running such test with opencl-c.h.

I would also like to loop @yaxunl in for the extra pair of eyes wrt prior to OpenCL 3 functionality.

Ideally, it would be good if we don't commit it too close to the release branch i.e. good to leave a few weeks for the bugs to be discovered and fixed considering the possible propagation time to the external projects.

@airlied I think this patch is outdated as some features have been committed in the meantime. Are you still interested in working on this change? It might help splitting into smaller independent chunks. It is usually faster to review and test.

I'll have to rebase/rebuild my build env, I'll try and have something rebasing this cleaner soon.

I'll have to rebase/rebuild my build env, I'll try and have something rebasing this cleaner soon.

Cool, thanks! If you could split/partition into multiple patches in some way it would be good. FYI the clang 13 release branch will be taken in a few weeks (before end of July!) so we still have time to get this into the upcoming release.