Page MenuHomePhabricator

[CMake] Detect information about test compiler
ClosedPublic

Authored by Hahnfeld on Nov 15 2017, 8:13 AM.

Details

Summary

Perform a nested CMake invocation to avoid writing our own parser
for compiler versions when we are not testing the in-tree compiler.
Use the extracted information to mark a test as unsupported that
hangs with Clang prior to version 4.0.1 and restrict tests for
libomptarget to Clang version 6.0.0 and later.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Nov 15 2017, 8:13 AM
Hahnfeld updated this revision to Diff 123039.Nov 15 2017, 9:07 AM

Fix stale variable from previous tries.

Hahnfeld updated this revision to Diff 123052.Nov 15 2017, 10:17 AM

Fix broken (inverted) logic of previous update and use LLVM_VERSION.

Hahnfeld updated this revision to Diff 123999.Nov 22 2017, 11:53 AM

Rebase onto recent changes.

This revision is now accepted and ready to land.Nov 29 2017, 12:26 PM

LGTM.

Thanks for the review! I'm going to wait until I got one round of testing by the build bots for the refactoring patches though.

Why do we limit the test compiler to be the same as the build compiler for libomp?
CMAKE_TEST_<lang>_COMPILER option would be useful to be able to run tests with different compilers.

I hope there is no such restriction in this patch: You should still be able to set OPENMP_TEST_C_COMPILER and OPENMP_TEST_CXX_COMPILER as before. To get their versions I implemented a nested CMake invocation where -DCMAKE_C_COMPILER=${OPENMP_TEST_C_COMPILER} -DCMAKE_CXX_COMPILER=${OPENMP_TEST_CXX_COMPILER} so that we don't have to maintain a version parser for all supported compilers.

The only restriction I added is a check that C and C++ compiler have the same ID - which was assumed anyway and could lead to nasty problems if the user passes in something different.

You should still be able to set OPENMP_TEST_C_COMPILER and OPENMP_TEST_CXX_COMPILER as before.

I failed to specify a test compiler with previous LIBOMP_TEST_* staff because I tried -DLIBOMP_TEST_C_COMPILER=gcc but it actually should have been -DLIBOMP_TEST_COMPILER=gcc. Somehow the old macro had _<lang>_ part missed ;)
It works for me now and I believe it will work for OPENMP_TEST_* too.
Thank you!

You should still be able to set OPENMP_TEST_C_COMPILER and OPENMP_TEST_CXX_COMPILER as before.

I failed to specify a test compiler with previous LIBOMP_TEST_* staff because I tried -DLIBOMP_TEST_C_COMPILER=gcc but it actually should have been -DLIBOMP_TEST_COMPILER=gcc. Somehow the old macro had _<lang>_ part missed ;)
It works for me now and I believe it will work for OPENMP_TEST_* too.
Thank you!

Yes, I fixed that inconsistency in the previous refactoring. But I'm a bit confused because LIBOMP_TEST_* shouldn't work anymore?

You should still be able to set OPENMP_TEST_C_COMPILER and OPENMP_TEST_CXX_COMPILER as before.

I failed to specify a test compiler with previous LIBOMP_TEST_* staff because I tried -DLIBOMP_TEST_C_COMPILER=gcc but it actually should have been -DLIBOMP_TEST_COMPILER=gcc. Somehow the old macro had _<lang>_ part missed ;)
It works for me now and I believe it will work for OPENMP_TEST_* too.
Thank you!

Yes, I fixed that inconsistency in the previous refactoring. But I'm a bit confused because LIBOMP_TEST_* shouldn't work anymore?

I only confirm that the old LIBOMP_TEST* works if I use the correct macro name :).
I didn't check the new OPENP_TEST_* staff.

I only confirm that the old LIBOMP_TEST* works if I use the correct macro name :).
I didn't check the new OPENP_TEST_* staff.

Ok, do you then have more comments on this patch or can I go ahead and commit the change?

omalyshe accepted this revision.Nov 30 2017, 8:24 AM

LGTM.

This revision was automatically updated to reflect the committed changes.