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

Mon Nov 30 09:41:51 PST 2020

Anastasia added a comment.

In D92004#2420121 <https://reviews.llvm.org/D92004#2420121>, @svenvh wrote:

>> 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 <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? I anticipate it is likely we will have follow up fix ups.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92004