Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Enable MultiConfig builds #1184

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

alliepiper
Copy link
Collaborator

@alliepiper alliepiper commented Jun 8, 2020

Fixes #1159.

Also fixes or works around these issues:

Summary:

  • Bump CMake requirement to 3.15
    • Needed for CUDA_COMPILER_ID generator expression.
    • Removed workarounds for older CMake versions.
  • Removed warning flag specific to for C++98.
  • Dialects are now configured through target properties. Add new
    THRUST_CPP_DIALECT option for single config mode, and remove logic
    that modified CMAKE_CXX_STANDARD and CMAKE_CUDA_STANDARD.
  • Move testing related CMake code to testing/CMakeLists.txt
  • Move example related CMake code to examples/CMakeLists.txt
  • Move header testing related CMake code to
    cmake/ThrustHeaderTesting.cmake
  • Move CUDA configuration logic to cmake/ThrustCUDAConfig.cmake.
  • Explicitly include(cmake/*.cmake) files rather than searching
    CMAKE_MODULE_PATH -- we only want to use the ones in the repo.
  • Added ThrustMultiConfig.cmake
    • Handle MultiConfig (and single config) logic.
  • Added ThrustBuildTargetList.cmake
    • Builds the THRUST_TARGETS list, which contains one interface target
      for each enabled host/device/dialect configuration.
  • Added ThrustBuildCompilerTargets.cmake
    • Move warning flag, etc setup into it, bind compile interfaces to
      targets instead of global variables.
  • Removed common_variables.cmake and simplified FileCheck hooks.
    • Set THRUST_ENABLE_EXAMPLE_FILECHECK to turn on FileCheck validation
    • Automatically detect and set THRUST_FILECHECK_EXECUTABLE if installed
      and filechecking is enabled.
  • Removed THRUST_TREAT_FILE_AS_CXX
    • This worked by setting a cmake SOURCE_FILE property, which no longer
      works since multiconfig may build the same source file with both CXX
      and CUDA.
    • Instead, the .cu files are wrapped in a .cpp file that does
      nothing but include the .cu file. The .cpp files are then added
      to the CXX targets as sources.
    • See cmake/ThrustUtilities.cmake for implementation.
  • Fix bug in thrust-config.cmake where an internal var was not cached as
    expected.

@alliepiper alliepiper requested review from brycelelbach and griwes June 8, 2020 18:47
@alliepiper alliepiper force-pushed the bug/github/multiconfig/1159 branch 2 times, most recently from 6d68e2d to 6f6f235 Compare June 9, 2020 23:28
@alliepiper alliepiper force-pushed the bug/github/multiconfig/1159 branch from 6f6f235 to c771718 Compare June 10, 2020 21:50
@alliepiper alliepiper added only: cmake CMake changes only. Doesn't need internal NVIDIA CI. testing: gpuCI passed Passed gpuCI testing. labels Jun 10, 2020
@alliepiper alliepiper force-pushed the bug/github/multiconfig/1159 branch from c771718 to 578e2c3 Compare June 11, 2020 18:38
@brycelelbach
Copy link
Collaborator

Can we rename run_example.cmake and run_test.cmake to be consistent with the other CMake files in cmake/?

@brycelelbach
Copy link
Collaborator

We should probably avoid using "meta" in the context of multiconfig builds to avoid confusion with C++ metaprogramming.

@brycelelbach
Copy link
Collaborator

Previously the default output location for the binaries was the top level build directory; now the binaries are in testing and examples. Should we put them back in the top level build directory? Or in a bin directory? Same question for the test framework library.

# TBB/OMP | F | Not useful | Cheap | Mixes CPU-parallel systems
# OMP/TBB | F | Not useful | Cheap | Mixes CPU-parallel systems
# TBB/CPP | F | Not Useful | Cheap | Parallel host, serial device
# OMP/CPP | F | Not Useful | Cheap | Parallel host, serial device
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this in one of the top-level .md documents, like README?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a section on MultiConfig to CONTRIBUTING.md at some point, either in this patch or #1163 (since CONTRIBUTING doesn't exist yet).

Comment on lines +6 to +11
function(thrust_wrap_cu_in_cpp cpp_file_var cu_file thrust_target)
thrust_get_target_property(prefix ${thrust_target} PREFIX)
set(wrapped_source_file "${CMAKE_CURRENT_SOURCE_DIR}/${cu_file}")
set(cpp_file "${CMAKE_CURRENT_BINARY_DIR}/${prefix}/${cu_file}.cpp")
configure_file("${Thrust_SOURCE_DIR}/cmake/wrap_source_file.cpp.in" "${cpp_file}")
set(${cpp_file_var} "${cpp_file}" PARENT_SCOPE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@griwes any concerns about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tl;dr behind this is that we can't set the "treat as cpp" flag as a source file property anymore, since each source file is now compiled multiple times for multiple targets, some of which may still need to treat the file as CUDA.

Instead, a .cpp file is generated that just does #include <thrust/original/file.cu>, and that cpp file is compiled instead of the .cu.

@alliepiper
Copy link
Collaborator Author

alliepiper commented Jun 22, 2020

Previously the default output location for the binaries was the top level build directory; now the binaries are in testing and examples. Should we put them back in the top level build directory? Or in a bin directory? Same question for the test framework library.

This is super easy to change by setting this variable (or the equivalent target property) https://cmake.org/cmake/help/v3.15/variable/CMAKE_RUNTIME_OUTPUT_DIRECTORY.html

I like putting them in bin but can adapt to any of these suggestions (including leaving them where they are). Does anyone have a strong feeling on either of these choices? I'll set up the standard <build-dir>/bin/, lib/ etc locations unless anyone objects.

Fixes NVIDIA#1159.

Also fixes or works around these issues:
- WAR NVIDIA#1167:
  - Disable arches 53, 62, 72 for RDC-required tests, print warning
  - Error when ENABLE_RDC for tests/examples is set with a no-RDC arch.
- Fixes NVIDIA#1168: Set RUN_SERIAL on OpenMP and TBB tests.
- WAR ccache/ccache#598:
  nvcc flags `s/-Werror all-warnings/-Xcudafe --promote_warnings/g`
- WAR NVIDIA#1174: remove warning promotion from tbb.cuda targets.
- WAR NVIDIA#976: Add options to enable/disable tests, examples, header_tests:
  - THRUST_ENABLE_TESTING
  - THRUST_ENABLE_EXAMPLES
  - THRUST_ENABLE_HEADER_TESTING

Summary:
- Bump CMake requirement to 3.15
  - Needed for CUDA_COMPILER_ID generator expression.
  - Removed workarounds for older CMake versions.
- Removed warning flag specific to for C++98.
- Dialects are now configured through target properties. Add new
  THRUST_CPP_DIALECT option for single config mode, and remove logic
  that modified CMAKE_CXX_STANDARD and CMAKE_CUDA_STANDARD.
- Move testing related CMake code to `testing/CMakeLists.txt`
- Move example related CMake code to `examples/CMakeLists.txt`
- Move header testing related CMake code to
    `cmake/ThrustHeaderTesting.cmake`
- Move CUDA configuration logic to `cmake/ThrustCUDAConfig.cmake`.
- Explicitly `include(cmake/*.cmake)` files rather than searching
    CMAKE_MODULE_PATH -- we only want to use the ones in the repo.
- Added ThrustMultiConfig.cmake
  - Handle MultiConfig (and single config) logic.
- Added ThrustBuildTargetList.cmake
  - Builds the THRUST_TARGETS list, which contains one interface target
      for each enabled host/device/dialect configuration.
- Added ThrustBuildCompilerTargets.cmake
  - Move warning flag, etc setup into it, bind compile interfaces to
      targets instead of global variables.
- Renamed common_variables.cmake to ThrustCommonVariables.cmake
- Removed THRUST_TREAT_FILE_AS_CXX
  - This worked by setting a cmake SOURCE_FILE property, which no longer
    works since multiconfig may build the same source file with both CXX
    and CUDA.
  - Instead, the `.cu` files are wrapped in a `.cpp` file that does
    nothing but include the `.cu` file. The `.cpp` files are then added
    to the CXX targets as sources.
  - See `cmake/ThrustUtilities.cmake` for implementation.
- Fix bug in thrust-config.cmake where an internal var was not cached as
  expected.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
only: cmake CMake changes only. Doesn't need internal NVIDIA CI. testing: gpuCI passed Passed gpuCI testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenMP and TBB tests should run serially Enable multiconfig builds for testing / validation
2 participants