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

Fri Nov 30 13:50:54 PST 2018

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

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.

================
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
----------------
aganea wrote:
> rnk wrote:
> > 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.
> > 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?
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.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55056