[PATCH] D97869: [OpenCL][Draft] Add OpenCL builtin test generator

Wed Mar 10 06:30:18 PST 2021

Anastasia added a comment.

Thanks for the comprehensive summary. I don't have many other suggestions in mind but I would like to add a few thoughts.

We could always consider writing the tests manually from scratch too. But it doesn't feel like a good cost/benefit trade-off.

Regarding 2 and 4 I think we should drive towards deprecation of `opencl-c.h` as it is a maintenance overhead but we could convert it into a test instead?

Aside from option 3 all other options will require adding non-standard testing formats in Clang. This means that if there is any core refactoring changes even outside of OpenCL affecting the functionality in the headers, it would be required to learn how we do the testing in order to address any breakages. Perhaps this can be mitigated by the documentation, but overall it is an extra burden to the community. Speaking from my personal experience I was in the situations breaking functionality outside of OpenCL and having a unified testing format is a big advantage in understanding and addressing the issues in unfamiliar code.

>   The output is big, currently already 56k lines, and I expect it will still get a bit bigger when we add conditionals. I don't feel comfortable about putting such big autogenerated files under git (regardless of whether we add it as a single file, or as a collection of smaller files). Ideally, the repo should only contain the source from which we generate things.

What problems do you see with checking in autogenerated files? There are some existing tests that check similar functionality and they seem to contain repetitive patterns. Honestly, I wouldn't be able to say if they were written with copy/paste or auto-generated but I am not sure whether it is very important. Of course, it would have been better if we have built up the functionality incrementally, and it is still an option. But it hasn't been possible in the past and as a matter of fact the original header was committed as one large file (around 15K lines).

>   Updates to OpenCLBuiltins.td will give large diffs when regenerating the tests (regardless of whether the file is split or not).

Could you provide more details. I don't follow this.

>   Manually verifying that the generated test is actually correct is not trivial.

I think the way to approach this would be to just assume that the test is correct and then fix any issues we discover as we go along with the development. There is nothing wrong with this though as even small manually written tests have bugs.

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

https://reviews.llvm.org/D97869