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

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

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

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

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

Let's just make this OFF by default.

358

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.

cmake/modules/HandleLLVMOptions.cmake

   append("/Zc:inline" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
   # Generate PDB even in release for profiling.
-  if (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE")
+  option(LLVM_ENABLE_GEN_PDB ON)
+  if (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE" AND LLVM_ENABLE_GEN_PDB)
     append("/Zi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
-    append("/DEBUG" CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+    append("/DEBUG /OPT:REF /OPT:ICF" CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
   endif (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE")
 
   # /Zc:strictStrings is incompatible with VS12's (Visual Studio 2013's)