[CMake][benchmark] Cache result of CXXFeatureCheck

[beanz]

Summary

This check is actually pretty expensive. When cross-compiling it compiles and links, when not cross compiling it compiles, links and runs. By not caching this value we re-run the test every time cmake gets re-invoked.

In my local tests this change reduced iterative CMake executions from ~10.2 seconds to ~9.7 seconds.

[compnerd]

In general, yes, these checks can be expensive. I wish that there was a way to ensure that the naming of the features was consistent across all the projects so that the same tests do not need to be rerun.

llvm/utils/benchmark/cmake/CXXFeatureCheck.cmake

56

Mind using ON here?

64

Mind using OFF here?

[beanz]

Updating based on review feedback.

[lebedev.ri]

Please can you submit these changes upstream first?
(There are two other copies of benchmark - in libcxx, and test-suite)

[lebedev.ri]

Also, it's not a great idea to store something into cache/global scope without ensuring unique global prefix, so please change HAVE_ to BENCHMARK_HAVE_

[beanz]

@lebedev.ri I actually disagree with prefixing capabilities check variables. If other projects are checking the same thing you want them to use the same variable so that they don't all check the same thing over and over again.

Also, why is this duplicated between libcxx, test-suite and llvm? There is functionality in the libcxx build that requires the LLVM CMake modules (like building the tests), so it seems like a better solution would be having this module be a llvm CMake module that is shared across the other projects.

[lebedev.ri]

In D61387#1486871, @beanz wrote:

@lebedev.ri I actually disagree with prefixing capabilities check variables. If other projects are checking the same thing you want them to use the same variable so that they don't all check the same thing over and over again.

Or they happen to just use the same name but use it for different things.

Also, why is this duplicated between libcxx, test-suite and llvm? There is functionality in the libcxx build that requires the LLVM CMake modules (like building the tests), so it seems like a better solution would be having this module be a llvm CMake module that is shared across the other projects.

I'm not sure i follow.
There are three copies of the entire benchmark library - one here in llvm, one in libc++ and one in test-suite.
This isn't just about this cmake file (which is part of the benchmark library)

[beanz]

That kinda makes it worse. We don't duplicate gtest in all the tools that use it, why are we duplicating this library?

[lebedev.ri]

In D61387#1487036, @beanz wrote:

That kinda makes it worse. We don't duplicate gtest in all the tools that use it, why are we duplicating this library?

Testsuite can be built without llvm; llvm can be built without testsuite; llvm can be built without libc++ - so you already have two copies - in llvm and testsuite
Likely libc++ also wants to be buildable standalone (including benchmarks)?

[beanz]

Libc++'s test cases aren't buildable without LLVM. Why should the benchmarks be?

[lebedev.ri]

In D61387#1488279, @beanz wrote:

Libc++'s test cases aren't buildable without LLVM. Why should the benchmarks be?

Note that libc++'s benchmark predates llvm's benchmark,
so if libc++ really doesn't care about building benchmarks in standalone mode,
then i suppose libc++ benchmark is the one you want to drop.
But this isn't particularly relevant to this patch, i suppose.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

llvm/utils/benchmark/cmake/CXXFeatureCheck.cmake

 endif()
 set(__cxx_feature_check INCLUDED)
 
 function(cxx_feature_check FILE)
   string(TOLOWER ${FILE} FILE)
   string(TOUPPER ${FILE} VAR)
   string(TOUPPER "HAVE_${VAR}" FEATURE)
   if (DEFINED HAVE_${VAR})
-    set(HAVE_${VAR} 1 PARENT_SCOPE)
+    if(HAVE_${VAR})
       add_definitions(-DHAVE_${VAR})
+    endif()
     return()
   endif()
 
   if (NOT DEFINED COMPILE_${FEATURE})
     message("-- Performing Test ${FEATURE}")
     if(CMAKE_CROSSCOMPILING)
       try_compile(COMPILE_${FEATURE}
               ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/cmake/${FILE}.cpp
else()
               ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/cmake/${FILE}.cpp
               CMAKE_FLAGS ${BENCHMARK_CXX_LINKER_FLAGS}
               LINK_LIBRARIES ${BENCHMARK_CXX_LIBRARIES})
     endif()
   endif()
 
   if(RUN_${FEATURE} EQUAL 0)
     message("-- Performing Test ${FEATURE} -- success")
-    set(HAVE_${VAR} 1 PARENT_SCOPE)
+    set(HAVE_${VAR} ON CACHE BOOL "CXX feature test")
     add_definitions(-DHAVE_${VAR})
   else()
     if(NOT COMPILE_${FEATURE})
       message("-- Performing Test ${FEATURE} -- failed to compile")
     else()
       message("-- Performing Test ${FEATURE} -- compiled but failed to run")
     endif()
+    set(HAVE_${VAR} OFF CACHE BOOL "CXX feature test")
   endif()
 endfunction()