[PATCH] D55056: [CMake] Default options for faster executables on MSVC

[Reid Kleckner via Phabricator via llvm-commits]

rnk added subscribers: hans, zturner.
rnk added a comment.

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.



================
Comment at: llvm/trunk/cmake/modules/ChooseMSVCCRT.cmake:70-71
+
+	  # Make /MT the default in Release builds to make them faster
+	  # and avoid the DLL function thunking.
+	  if ((${build} STREQUAL "MINSIZEREL") OR
----------------
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.


================
Comment at: llvm/trunk/cmake/modules/HandleLLVMOptions.cmake:387
+      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()
----------------
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55056/new/

https://reviews.llvm.org/D55056