Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSVC CMake generation optimization (13.6s -> 5.7s) #2852

Merged

Conversation

mattiasljungstrom
Copy link
Contributor

@mattiasljungstrom mattiasljungstrom commented Apr 6, 2022

The following CMake changes reduce project generation on Win11 with MSVC 2022 from 13.6s to 5.7s. (Using a deskop system with Ryzen 3900X / M.2 SSD).

Motivation:

  • Tests in cxx14.cmake are using Clang/GCC style flags, these are all invalid on MSVC.
  • The flag SUPPORTS_USER_DEFINED_LITERALS is only used for tests that are disabled on WIN32.
  • The flag FMT_HAS_VARIANT is not used at all from what I can see. (?)
  • check_cxx_compiler_flag(-fno-exceptions) is always invalid on MSVC.
  • check_cxx_compiler_flag(-fno-delete-null-pointer-checks) is always invalid on MSVC.

With FMT_TEST=OFF, project generation takes 4.3s and is almost instant when included as sub-project.

Note, correct check on MSVC would be like this:

check_cxx_compiler_flag("/std:c++20" has_std_20_flag)

But it might be better to use CMake checks instead of testing flags, like 'CMAKE_CXX_COMPILE_FEATURES' (available from CMake 3.1).

@mattiasljungstrom mattiasljungstrom changed the title MSVC CMake generation optimization MSVC CMake generation optimization (13.6s -> 5.7s) Apr 6, 2022
@mattiasljungstrom
Copy link
Contributor Author

this style also works on MSVC: (note ':' instead of '=')

check_cxx_compiler_flag(-std:c++20 has_std_20_flag)

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Instead of patching every invocation of check_cxx_compiler_flag let's move it to one place, a new fmt_check_cxx_compiler_flag function, that is a noop for MSVC.

@vitaut
Copy link
Contributor

vitaut commented Apr 6, 2022

The flag FMT_HAS_VARIANT is not used at all from what I can see.

Let's remove the check completely then.

@mattiasljungstrom mattiasljungstrom force-pushed the msvc_cmake_gen_optimization branch from 1285773 to 15da7c2 Compare April 7, 2022 11:00
@mattiasljungstrom
Copy link
Contributor Author

Pushed an update with your change requests.

Note: it's possible to run Clang under MSVC, in which case some clang/gcc flags are supported. But not the "-std=c++XX" flags. These still use the MSVC format instead.

endif()
message(STATUS "CXX_STANDARD: ${CMAKE_CXX_STANDARD}")
endif ()
message(STATUS "CMAKE_CXX_STANDARD: ${CMAKE_CXX_STANDARD}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, please revert.

include(CheckCXXCompilerFlag)
macro (fmt_check_cxx_compiler_flag _FLAG _RESULT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it a function instead of a macro unless there is a strong reason not to and use normal naming convention, i.e. flag and result for arguments.

Comment on lines 56 to 65
if (FMT_PEDANTIC AND NOT WIN32)
include(CheckCXXSourceCompiles)
set(CMAKE_REQUIRED_FLAGS ${CXX_STANDARD_FLAG})
# Check if user-defined literals are available
check_cxx_source_compiles("
void operator\"\" _udl(long double);
int main() {}"
SUPPORTS_USER_DEFINED_LITERALS)
set(CMAKE_REQUIRED_FLAGS )
endif ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to test/compile-error-test/CMakeLists.txt and make unconditional. The other usage of SUPPORTS_USER_DEFINED_LITERALS us unnecessary and can be removed.

@mattiasljungstrom mattiasljungstrom force-pushed the msvc_cmake_gen_optimization branch from 15da7c2 to bef89c9 Compare April 8, 2022 09:42
@mattiasljungstrom
Copy link
Contributor Author

Pushed a new update based on your feedback.

@@ -171,8 +167,7 @@ if (FMT_PEDANTIC AND NOT WIN32)
"-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
"-DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}"
"-DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD}"
"-DFMT_DIR=${CMAKE_SOURCE_DIR}"
"-DSUPPORTS_USER_DEFINED_LITERALS=${SUPPORTS_USER_DEFINED_LITERALS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

CXX_STANDARD_FLAG is not defined in test/compile-error-test/CMakeLists.txt

Suggestion:
+ "-DCXX_STANDARD_FLAG=${CXX_STANDARD_FLAG}"

@mattiasljungstrom mattiasljungstrom force-pushed the msvc_cmake_gen_optimization branch from bef89c9 to bcfca65 Compare April 8, 2022 10:35
@vitaut vitaut merged commit a935ac3 into fmtlib:master Apr 8, 2022
@vitaut
Copy link
Contributor

vitaut commented Apr 8, 2022

Thank you

@mattiasljungstrom mattiasljungstrom deleted the msvc_cmake_gen_optimization branch April 9, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants