Allow disabling PDB generation in Release build

[rnk]

Summary

https://reviews.llvm.org/D42632 increases my CI build time from "completed in 53 minutes" to "only finished building [1492 / 1603] (93%) in 1 hour".

Pass -DLLVM_ENABLE_PDB=OFF if you do not want PDB.

Drive-by-fix: add /OPT:REF and /OPT:ICF after /DEBUG.

[takuto.ikuta]

cmake/modules/HandleLLVMOptions.cmake

357 ↗

(On Diff #133743)

Can I ask you to use option(LLVM_ENABLE_PDB ON) instead?
I think LLVM_ENABLE_PDB in if condition is easy to understand than NOT LLVM_DISABLE_PDB.

[rongjiecomputer]

I used LLVM_DISABLE_PDB as that was the name suggested in https://reviews.llvm.org/D42632#990897, but I agree that the if statement becomes harder to read. Done.

[takuto.ikuta]

cmake/modules/HandleLLVMOptions.cmake

360 ↗

(On Diff #133775)

Can I ask you to change /OPT:NOICF here also, recommended by docs?
https://docs.microsoft.com/en-us/cpp/build/reference/opt-optimizations#arguments
/DEBUG in release build is for profiling, and I want to preserve original function name in stacktrace.

[rongjiecomputer]

cmake/modules/HandleLLVMOptions.cmake

360 ↗

(On Diff #133775)

But won't that penalize everyone with bigger binary size (due to less folding) if they do not know LLVM_ENABLE_PDB flag exists?

[takuto.ikuta]

LGTM, thank you for follow up.

cmake/modules/HandleLLVMOptions.cmake

360 ↗

(On Diff #133775)

I see. If ICF is problem on profiled result, I'll disable it in my local or introduce another option.

[rongjiecomputer]

Can someone help me to land this, or I still more LGTMs from other reviewers?

[takuto.ikuta]

Nico, can I ask you to land this?

[rnk]

I don't think we should enable PDBs in release mode by default. One of the main uses cases for building in release mode (without PDBs) is to speed up the build. /Zi creates bottlenecks on machines with many cores, and adding /debug to the final link step really hurts incremental build speed.

We've been building LLVM in release mode with PDBs in Chromium for a while now by manually tweaking flags outside the LLVM build system:
https://cs.chromium.org/chromium/src/tools/clang/scripts/update.py?q=update.py+clang&sq=package:chromium&dr=CSs&l=597

If we want to make it easier to profile LLVM, we should add some high level option like LLVM_ENABLE_PROFILING and put these flag tweaks under there. Once you have that, then that's a reasonable place to disable ICF.

[thakis]

So should we revert https://reviews.llvm.org/D42632 for now?

[rongjiecomputer]

What about setting LLVM_ENABLE_PDB off by default and change /OPT:ICF to /OPT:NOICF?

[rnk]

cmake/modules/HandleLLVMOptions.cmake

357 ↗

(On Diff #133775)

Let's just make this OFF by default.

358 ↗

(On Diff #133775)

I'd switch the condition order, so we check if we're enabling PDBs first, then look at the build type.

There are other build types that we might wish to handle later, but just doing this in Release is fine for now.

[rnk]

I'll go ahead and make these changes to get us back to our behavior before https://reviews.llvm.org/D42632.

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

if( MSVC )
   )
 
   if (LLVM_ENABLE_WERROR)
     append("/WX" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endif (LLVM_ENABLE_WERROR)
 
   append("/Zc:inline" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
-  # Generate PDB even in release for profiling.
-  if (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE")
+  # Allow users to request PDBs in release mode. CMake offeres the
+  # RelWithDebInfo configuration, but it uses different optimization settings
+  # (/Ob1 vs /Ob2 or -O2 vs -O3). LLVM provides this flag so that users can get
+  # PDBs without changing codegen.
+  option(LLVM_ENABLE_PDB OFF)
+  if (LLVM_ENABLE_PDB AND uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE")
     append("/Zi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
-    append("/DEBUG" CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
-  endif (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE")
+    # /DEBUG disables linker GC and ICF, but we want those in Release mode.
+    append("/DEBUG /OPT:REF /OPT:ICF"
+          CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS
+          CMAKE_SHARED_LINKER_FLAGS)
+  endif()
 
   # /Zc:strictStrings is incompatible with VS12's (Visual Studio 2013's)
   # debug mode headers. Instead of only enabling them in VS2013's debug mode,
   # we'll just enable them for Visual Studio 2015 (VS 14, MSVC_VERSION 1900)
   # and up.
   if (NOT (MSVC_VERSION LESS 1900))
     # Disable string literal const->non-const type conversion.
     # "When specified, the compiler requires strict const-qualification