[Openmp-commits] [PATCH] D51687: [libomptarget-nvptx] Add testing infrastructure

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

In https://reviews.llvm.org/D51687#1230375, @gtbercea wrote:

> Considering your comment in the description about requiring latest Clang perhaps you should revisit this patch: https://reviews.llvm.org/D46842


In my opinion the two matters are different with regard to an important detail:

1. The linked change in https://reviews.llvm.org/D46842 is about **building** libomptarget-nvptx with in-tree Clang. As a result the build system would recompile the library whenever there is a change in the Clang sources. For me this drawback outweights the potential gain; in particular you can build the bclib with Clang 7.0 and it will still be inlined when building an application with a later version of Clang (e.g. current trunk), so I don't see the need to use in-tree Clang.
2. This patch adds **testing** infrastructure which will kick in after libomptarget-nvptx has already been built. At the moment the tests will only work with the latest Clang because it relies on `--libomptarget-nvptx-path` and code generation that is not yet included in a released version of LLVM. Incidentally I'm listing this as one of the reasons why `check-libomptarget-nvptx` is not included in neither `check-all` nor `check-openmp`. (However I think "we can't determine if there is a GPU plugged into the system" is the one that really matters and we'll probably never be able to solve this, so I don't see enabling these tests by default anyway.)

I hope this clarifies my thoughts that I've brought up in some review threads. From my point of view there is no contradiction to what I propose in this patch and for me the choice of not enabling the tests by default is the logical consequence of my previous statements.


https://reviews.llvm.org/D51687