[CMake] Default options for faster executables on MSVC

[aganea]

Summary

This patch:

• Disables incremental linking by default. /INCREMENTAL adds extra thunks in the EXE, which makes execution slower.
• Sets /MT (static CRT lib) by default instead of CMake's default /MD (dll CRT lib). The previous default /MD makes all DLL functions to be thunked, thus making execution slower (memcmp, memset, etc.)
• Adds LLVM_ENABLE_INCREMENTAL_LINK which is set to OFF by default.

DLL thunks:

/INCREMENTAL thunks:

I thought it'd be better if Clang / LLVM / LLD would be fast out-of-the-box, instead of people having to dig for "good" settings.

Below are timings for linking a large DLL with LLD. The type merging pass is dominant, and is affected by those two options, because of the tight merge loop. I currently have several optimisations in that loop that tend to increase the gap.

Without this patch (default cmake settings):

Type Merging:           23174 ms ( 56.9%)

With this patch (new default cmake settings):

Type Merging:           22338 ms ( 52.2%)

This is a MSVC-only change. Tested with VS2017 15.9.1 (MSVC 19.16.27023.1 x64)

[rnk]

So, one thing to keep in mind is that RelWithDebInfo is not what it looks like. If you are like me, you might think that it enables the same optimizations as a Release build and enables debug info. Unfortunately, that's incorrect. You've noticed that it uses the /INCREMENTAL link flag, but more importantly, it adds the /Ob1 compile flag, which forbids the compiler from inlining functions not specifically marked inline. This means there are *significant* performance differences between a plain Release build and a RelWithDebInfo build, so it's not useful for profiling. To deal with this, @zturner added the LLVM_ENABLE_PDB option.

I suppose if we want we can go down this path of trying to take things into our own hands and overriding the cmake defaults, but it seemed better to sidestep the issue and keep building in the Release configuration, which already passes /INCREMENTAL:NO. But, maybe if you're building from the IDE, I could see why you'd prefer to make RelWithDebInfo behave the way we expect it to.

llvm/trunk/cmake/modules/ChooseMSVCCRT.cmake

70–71

Isn't it a bad practice to statically link the CRT into DLLs? We have a few (LLDB), and this will get picked up by it. However, I notice that we set this option in the official build_llvm_package.bat script that @hans uses to make the Windows releases:
https://github.com/llvm-git-prototype/llvm/blob/master/llvm/utils/release/build_llvm_package.bat#L47

Anyway, this is just the default, and it's reasonable for people building clang, lld, and other static executables, which is most people.

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

387

Looks like it needs one more indentation level to be in the loop over target types. We don't have a formatter tool for cmake, but it would be easier to read if this was broken up onto multiple lines.

[zturner]

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

387

Instead of replacing all the options in an existing command line, why don't we just have something like:

if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC" AND NOT LLVM_ENABLE_INCREMENTAL_LINK AND CMAKE_BUILD_TYPE != "Debug")
  append("/INCREMENTAL:NO" CMAKE_LINKER_FLAGS)
endif()

[aganea]

Indent.

llvm/trunk/cmake/modules/ChooseMSVCCRT.cmake

70–71

Isn't it a bad practice to statically link the CRT into DLLs?

Yes, only if you dynamically load (third-party) DLLs in your executable at runtime (plugins say) The EXE and DLLs might be compiled with a different CRTs and you might end up accessing wrong class/structure layouts in memory.

However only LTO seems to be compiled as a DLL. I'm not sure why, and how it's used? Is it supposed to be dynamically loaded by LLD?

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

387

(note: CMAKE_LINKER_FLAGS doesn't exist, rather CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS CMAKE_STATIC_LINKER_FLAGS)

I've tried several approches including the one you suggest, however replacing the flag (REGEX REPLACE) is the only one that works with my CMake 3.12.2. Probably because how /INCREMENTAL is forcibly set by CMake, and probably because how flags are then rendered into the .vcxproj by the generator.

[aganea]

In D55056#1314996, @rnk wrote:

So, one thing to keep in mind is that RelWithDebInfo is not what it looks like. If you are like me, you might think that it enables the same optimizations as a Release build and enables debug info. Unfortunately, that's incorrect. You've noticed that it uses the /INCREMENTAL link flag, but more importantly, it adds the /Ob1 compile flag, which forbids the compiler from inlining functions not specifically marked inline. This means there are *significant* performance differences between a plain Release build and a RelWithDebInfo build, so it's not useful for profiling. To deal with this, @zturner added the LLVM_ENABLE_PDB option.

I suppose if we want we can go down this path of trying to take things into our own hands and overriding the cmake defaults, but it seemed better to sidestep the issue and keep building in the Release configuration, which already passes /INCREMENTAL:NO. But, maybe if you're building from the IDE, I could see why you'd prefer to make RelWithDebInfo behave the way we expect it to.

Currently RelWithDebInfo is "the cake is a lie". It isn't exactly what you expect, and users might loose long hours (like I did) understanding why the build is slower, when compared to Release. You want RelWithDebInfo because you want to debug crashes in production (I wasn't aware of LLVM_ENABLE_PDB, thanks!).
This patch makes RelWithDebInfo almost equivalent to Release+PDB, except for /Ob1 (which we could also change to /Ob2 in RelWithDebInfo). In the past /Ob2 was harder to debug with MSVC; but since the addition of /d2Zi+ slash /Zo, optimized builds suddenly have friendlier debug streams.

[rnk]

I just chatted with @zturner, and we came to the conclusion that if you want to make RelWithDebInfo have the exact same optimizations as Release, we'd be in favor. Right now it's just a trap. I think the optimization level may be different on non-Windows platforms, maybe -O3 vs -O2 or -Os, but I've forgotten.

The MinSizeRel changes also seem reasonable.

llvm/trunk/cmake/modules/ChooseMSVCCRT.cmake

70–71

I believe it is. I think Sony uses it for their linker, which runs on Windows. I'm not aware of other users. LLD links LLVM statically (bloating the release package, but that's another story).

LLDB is also a Python module, which is basically a DLL. So, these are kind of bad defaults for those projects, but I think they are in the minority and whoever is building them can override this default. /MT is a good default for our released packages anyway, since it makes them more redistributable.

[aganea]

In D55056#1315222, @rnk wrote:

I just chatted with @zturner, and we came to the conclusion that if you want to make RelWithDebInfo have the exact same optimizations as Release, we'd be in favor. Right now it's just a trap. I think the optimization level may be different on non-Windows platforms, maybe -O3 vs -O2 or -Os, but I've forgotten.

The MinSizeRel changes also seem reasonable.

In that case can’t we just keep Debug and Release+PDB, and drop the two others?

[rnk]

In D55056#1315675, @aganea wrote:

In that case can’t we just keep Debug and Release+PDB, and drop the two others?

I guess I'm not sure what you mean. Do you mean, drop them, as in find a way to remove them from the list of default configurations so that they don't display in IDEs as a configuration option? That makes sense. But I imagine that IDE users would want some configuration for "Release+PDB" that doesn't require re-running cmake, and RelWithDebInfo seems like the right name for it.

[aganea]

In D55056#1317117, @rnk wrote:

In D55056#1315675, @aganea wrote:

In that case can’t we just keep Debug and Release+PDB, and drop the two others?

I guess I'm not sure what you mean. Do you mean, drop them, as in find a way to remove them from the list of default configurations so that they don't display in IDEs as a configuration option? That makes sense. But I imagine that IDE users would want some configuration for "Release+PDB" that doesn't require re-running cmake, and RelWithDebInfo seems like the right name for it.

The other way around: why would you keep Release without debug info?

[aganea]

The point being, I'd keep Debug & Release targets; drop RelWithDebInfo & MinSizeRel; and add debugging info (LLVM_ENABLE_PDB) to the Release target.

[zturner]

Some people like to build Release without debug info because debug info slows down the build time. Personally I think debug info should be always-on and we should use the increased build time as a way to motivate people to submit patches to make debug info generation faster, but that's just me :)

[rnk]

In D55056#1317122, @aganea wrote:

The point being, I'd keep Debug & Release targets; drop RelWithDebInfo & MinSizeRel; and add debugging info (LLVM_ENABLE_PDB) to the Release target.

Makes sense, but LLVM_ENABLE_PDB doesn't make sense on non-Windows OSs. We also have to consider that linking with debug info enabled on Linux is painfully slow, so it might make more sense to make it off by default. I learned to build in Release+Asserts on Linux specifically to speed up the build. As slow as it is to make PDBs with LLD, I promise you dsymutil and gold+objcopy are slower. =S

So, after considering all that, maybe it is best to have MSVC and clang-cl build diverge from the other platforms. If we think making PDBs by default is fast enough, then let's just enable them for all configurations and get rid of MinSizeRel and RelWithDebInfo.

[aganea]

In D55056#1317377, @rnk wrote:

Makes sense, but LLVM_ENABLE_PDB doesn't make sense on non-Windows OSs. We also have to consider that linking with debug info enabled on Linux is painfully slow, so it might make more sense to make it off by default. I learned to build in Release+Asserts on Linux specifically to speed up the build. As slow as it is to make PDBs with LLD, I promise you dsymutil and gold+objcopy are slower. =S

How do you debug Release builds if you need to? (on Linux let's say) Do you activate debug info only for the TU that needs debugging?

So, after considering all that, maybe it is best to have MSVC and clang-cl build diverge from the other platforms. If we think making PDBs by default is fast enough, then let's just enable them for all configurations and get rid of MinSizeRel and RelWithDebInfo.

link.exe is still slow, especially if /INCREMENTAL is disabled. Before going any further on discussing this, I'll do a round of compilation tests to compare with/without MinSizeRel/RelWithDebInfo.

[stella.stamenova]

This change caused a number of failures in the LLDB tests (as in 40+ failures) on Windows:

http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/2404/steps/test/logs/stdio

It looks like LLDB is occasionally failing to execute with exception 0xC0000409 (STATUS_STACK_BUFFER_OVERRUN). I think we should revert the change until we can correctly update the CMake files for LLDB unless it can be fixed quickly.

[aganea]

Reverted in r349654. I will take a look at the LLDB tests.

[aganea]

My bad - I reverted the wrong thing. Reverted for good in r349656.

[aganea]

Reopening with fixes for LLDB.

This build break was caused by the liblldb.dll used along with /MT (static CRT) which is usually not advisable. lldb.exe was initializing a handle for the stdout stream (using _pipe) and consuming it in liblldb.dll. /MT evidently creates internal states that are different across module bounds (exe, dll), which causes the stdout handle (__pioinfo in this case) to point to separate copies in memory.

[stella.stamenova]

cmake/modules/ChooseMSVCCRT.cmake

72 ↗

(On Diff #200578)

As far as I understand, /MT is being disabled because it is required by LLDB that it be disabled. Is that correct? If that's the case, I think the message needs to change to be clearer. For example: "Disabling /MT *as* required by LLDB". Right now it reads like /MT is being disabled but it is required by LLDB.

[aganea]

cmake/modules/ChooseMSVCCRT.cmake

72 ↗

(On Diff #200578)

As requested: "Disabling /MT as required by LLDB"

[rnk]

Previously I passed LLVM_USE_CRT_RELEASE=MT explicitly as part of my build, and I ran into the same problems with LLDB that you did. I'll try to modify this once it lands to warn when the user tries to do that. At the time, I couldn't see how to do it easily.

[aganea]

In D55056#1514451, @rnk wrote:

Previously I passed LLVM_USE_CRT_RELEASE=MT explicitly as part of my build, and I ran into the same problems with LLDB that you did. I'll try to modify this once it lands to warn when the user tries to do that. At the time, I couldn't see how to do it easily.

I can change this to (also) display the message when LLVM_USE_CRT_* is passed explicitly.

[aganea]

If using -DLLVM_TOOL_LLDB_BUILD it now displays "Using /MD as required by LLDB."
If using -DLLVM_TOOL_LLDB_BUILD -DLLVM_USE_CRT_RELEASE=MT it displays "Disabling /MT as required by LLDB."
Otherwise /MT is the new default.

[rnk]

lgtm

[stella.stamenova]

This caused failures on the windows bot again:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/5206/steps/test/logs/stdio

[dmajor]

For posterity: this was reverted in r361837.

llvm/trunk/CMakeLists.txt

 endif()
 option(LLVM_ENABLE_CXX1Y "Compile with C++1y enabled." OFF)
 option(LLVM_ENABLE_CXX1Z "Compile with C++1z enabled." OFF)
 option(LLVM_ENABLE_LIBCXX "Use libc++ if available." OFF)
 option(LLVM_ENABLE_LLD "Use lld as C and C++ linker." OFF)
 option(LLVM_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
 option(LLVM_ENABLE_WERROR "Fail and stop if a warning is triggered." OFF)
 
+if (MSVC)
+   option(LLVM_ENABLE_INCREMENTAL_LINK "Link incrementally. Enabling it might produce slower executables." OFF)
+endif()
+
 option(LLVM_ENABLE_DUMP "Enable dump functions even when assertions are disabled" OFF)
 
 if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" OFF)
 else()
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" ON)
 endif()
 

llvm/trunk/cmake/modules/ChooseMSVCCRT.cmake

variables (LLVM_USE_CRT_DEBUG, etc) instead.")
   make_crt_regex(MSVC_CRT_REGEX ${MSVC_CRT})
 
   foreach(build_type ${CMAKE_CONFIGURATION_TYPES} ${CMAKE_BUILD_TYPE})
     string(TOUPPER "${build_type}" build)
     if (NOT LLVM_USE_CRT_${build})
       get_current_crt(LLVM_USE_CRT_${build}
         MSVC_CRT_REGEX
         CMAKE_CXX_FLAGS_${build})
+
+    # Make /MT the default in Release builds to make them faster
+    # and avoid the DLL function thunking.
+    if ((${build} STREQUAL "MINSIZEREL") OR
+      (${build} STREQUAL "RELEASE") OR
+      (${build} STREQUAL "RELWITHDEBINFO"))
+      set(LLVM_USE_CRT_${build} "MT")
+    endif()
+
       set(LLVM_USE_CRT_${build}
         "${LLVM_USE_CRT_${build}}"
         CACHE STRING "Specify VC++ CRT to use for ${build_type} configurations."
         FORCE)
       set_property(CACHE LLVM_USE_CRT_${build}
         PROPERTY STRINGS ;${${MSVC_CRT}})
     endif(NOT LLVM_USE_CRT_${build})
   endforeach(build_type)

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

if( MSVC )
   endif(NOT (MSVC_VERSION LESS 1900))
 
   # "Generate Intrinsic Functions".
   append("/Oi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
   # "Enforce type conversion rules".
   append("/Zc:rvalueCast" CMAKE_CXX_FLAGS)
 
+  if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC" AND NOT LLVM_ENABLE_INCREMENTAL_LINK)
+    foreach(CONFIG RELEASE RELWITHDEBINFO MINSIZEREL)
+      foreach(FLAG EXE MODULE SHARED STATIC)
+      string(REGEX REPLACE "[-/](INCREMENTAL:YES|INCREMENTAL:NO|INCREMENTAL)" "/INCREMENTAL:NO" CMAKE_${FLAG}_LINKER_FLAGS_${CONFIG} "${CMAKE_${FLAG}_LINKER_FLAGS_${CONFIG}}")
+    endforeach()
+  endforeach()
+  endif()
+
   if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT LLVM_ENABLE_LTO)
     # clang-cl and cl by default produce non-deterministic binaries because
     # link.exe /incremental requires a timestamp in the .obj file.  clang-cl
     # has the flag /Brepro to force deterministic binaries. We want to pass that
     # whenever you're building with clang unless you're passing /incremental
     # or using LTO (/Brepro with LTO would result in a warning about the flag
     # being unused, because we're not generating object files).
     # This checks CMAKE_CXX_COMPILER_ID in addition to check_cxx_compiler_flag()