[PATCH] D92004: [OpenCL] add CL 3.0 optional feature support to opencl-c.h

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

>> 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 <https://github.com/KhronosGroup/SPIRV-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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92004/new/

https://reviews.llvm.org/D92004