Skip to content

Commit

Permalink
Correctly compile benchmarks (#7485)
Browse files Browse the repository at this point in the history
The refactor in #7107 introduced two subtle bugs that caused benchmarks to not build correctly.

1. The `benchmark_main` target needs to be explicitly placed on the final executable link line as it injects object files, and that doesn't work via transitive propagation.
2. The variable names `BUILD_BENCHMARKS` and `BUILD_TESTING` are shared between RMM and CUDF. When CUDF builds RMM via CPM it will force cache these variables to `OFF`. This means that if a developer try to run `cmake -DBUILD_BENCHMARKS=ON .` any subsequent execution of just `cmake .` will cause CUDF to forget it should have kept benchmarks enabled.

Authors:
  - Robert Maynard (@robertmaynard)

Approvers:
  - Keith Kraus (@kkraus14)

URL: #7485
  • Loading branch information
robertmaynard authored Mar 2, 2021
1 parent cf4c54d commit 41f2487
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 6 deletions.
4 changes: 2 additions & 2 deletions cpp/benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ target_link_libraries(cudf_datagen
GTest::gmock_main
GTest::gtest_main
benchmark::benchmark
benchmark::benchmark_main
Threads::Threads
cudf)

Expand All @@ -52,7 +51,8 @@ function(ConfigureBench CMAKE_BENCH_NAME)
add_executable(${CMAKE_BENCH_NAME} ${ARGN})
set_target_properties(${CMAKE_BENCH_NAME}
PROPERTIES RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${CUDF_BINARY_DIR}/gbenchmarks>")
target_link_libraries(${CMAKE_BENCH_NAME} PRIVATE cudf_benchmark_common cudf_datagen)
target_link_libraries(${CMAKE_BENCH_NAME}
PRIVATE cudf_benchmark_common cudf_datagen benchmark::benchmark_main)
endfunction()

###################################################################################################
Expand Down
18 changes: 17 additions & 1 deletion cpp/cmake/thirdparty/CUDF_GetRMM.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,26 @@
# limitations under the License.
#=============================================================================

function(find_and_configure_rmm VERSION)
function(cudf_save_if_enabled var)
if(CUDF_${var})
unset(${var} PARENT_SCOPE)
unset(${var} CACHE)
endif()
endfunction()

function(cudf_restore_if_enabled var)
if(CUDF_${var})
set(${var} ON CACHE INTERNAL "" FORCE)
endif()
endfunction()

function(find_and_configure_rmm VERSION)
# Consumers have two options for local source builds:
# 1. Pass `-D CPM_rmm_SOURCE=/path/to/rmm` to build a local RMM source tree
# 2. Pass `-D CMAKE_PREFIX_PATH=/path/to/rmm/build` to use an existing local
# RMM build directory as the install location for find_package(rmm)
cudf_save_if_enabled(BUILD_TESTS)
cudf_save_if_enabled(BUILD_BENCHMARKS)

CPMFindPackage(NAME rmm
VERSION ${VERSION}
Expand All @@ -32,6 +46,8 @@ function(find_and_configure_rmm VERSION)
"CMAKE_CUDA_ARCHITECTURES ${CMAKE_CUDA_ARCHITECTURES}"
"DISABLE_DEPRECATION_WARNING ${DISABLE_DEPRECATION_WARNING}"
)
cudf_restore_if_enabled(BUILD_TESTS)
cudf_restore_if_enabled(BUILD_BENCHMARKS)

if(NOT rmm_BINARY_DIR IN_LIST CMAKE_PREFIX_PATH)
list(APPEND CMAKE_PREFIX_PATH "${rmm_BINARY_DIR}")
Expand Down
4 changes: 1 addition & 3 deletions cpp/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ target_compile_features(cudftestutil PUBLIC cxx_std_14 cuda_std_14)
target_link_libraries(cudftestutil
PUBLIC GTest::gmock
GTest::gtest
GTest::gmock_main
GTest::gtest_main
Threads::Threads
cudf)

Expand All @@ -55,7 +53,7 @@ function(ConfigureTest CMAKE_TEST_NAME )
add_executable(${CMAKE_TEST_NAME} ${ARGN})
set_target_properties(${CMAKE_TEST_NAME}
PROPERTIES RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${CUDF_BINARY_DIR}/gtests>")
target_link_libraries(${CMAKE_TEST_NAME} PRIVATE cudftestutil)
target_link_libraries(${CMAKE_TEST_NAME} PRIVATE cudftestutil GTest::gmock_main GTest::gtest_main)
add_test(NAME ${CMAKE_TEST_NAME} COMMAND ${CMAKE_TEST_NAME})
endfunction()

Expand Down

0 comments on commit 41f2487

Please sign in to comment.