This is an archive of the discontinued LLVM Phabricator instance.

Differential D43156

Allow disabling PDB generation in Release build
ClosedPublic

Authored by rnk on Feb 9 2018, 11:46 PM.

Details

Reviewers
takuto.ikuta
thakis
zturner
rongjiecomputer
Commits
rGb97ff922a9ee: Don't make PDBs by default in Release mode
rL325296: Don't make PDBs by default in Release mode
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.

Diff Detail

Event Timeline

rongjiecomputer created this revision.Feb 9 2018, 11:46 PM
Herald added subscribers: llvm-commits, mgorny. · View Herald TranscriptFeb 9 2018, 11:46 PM
rongjiecomputer updated this revision to Diff 133743.Feb 10 2018, 12:07 AM
rongjiecomputer edited the summary of this revision. (Show Details)
takuto.ikuta added inline comments.Feb 10 2018, 3:22 PM
cmake/modules/HandleLLVMOptions.cmake
357–358

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 updated this revision to Diff 133775.Feb 10 2018, 4:08 PM
rongjiecomputer marked an inline comment as done.

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.

rongjiecomputer edited the summary of this revision. (Show Details)Feb 10 2018, 4:09 PM
takuto.ikuta added inline comments.Feb 10 2018, 4:41 PM
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 added inline comments.Feb 10 2018, 6:18 PM
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 accepted this revision.Feb 10 2018, 6:51 PM

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.

This revision is now accepted and ready to land.Feb 10 2018, 6:51 PM
rongjiecomputer added a comment.Feb 12 2018, 6:16 PM

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

takuto.ikuta added a reviewer: thakis.Feb 13 2018, 3:45 AM
takuto.ikuta added a comment.Feb 13 2018, 3:47 AM

Nico, can I ask you to land this?

rnk added a reviewer: zturner.Feb 13 2018, 10:16 AM
rnk added a comment.Feb 13 2018, 10:24 AM

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 added a comment.Feb 15 2018, 7:10 AM

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

rongjiecomputer added a comment.Feb 15 2018, 7:16 AM

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

rnk requested changes to this revision.Feb 15 2018, 11:14 AM
rnk added inline comments.
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.

This revision now requires changes to proceed.Feb 15 2018, 11:14 AM
rnk commandeered this revision.Feb 15 2018, 11:15 AM
rnk edited reviewers, added: rongjiecomputer; removed: rnk.

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

This revision is now accepted and ready to land.Feb 15 2018, 11:15 AM
Closed by commit rL325296: Don't make PDBs by default in Release mode (authored by rnk). · Explain WhyFeb 15 2018, 1:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
cmake/
modules/
7 lines

Diff 133775

cmake/modules/HandleLLVMOptions.cmake

Context not available.
append("/Zc:inline" CMAKE_C_FLAGS CMAKE_CXX_FLAGS) append("/Zc:inline" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
# Generate PDB even in release for profiling. # Generate PDB even in release for profiling.
if (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE") option(LLVM_ENABLE_PDB ON)
rnkAuthorUnsubmitted
Not Done
ReplyInline Actions

Let's just make this OFF by default.

rnk: Let's just make this OFF by default.
if (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE" AND LLVM_ENABLE_PDB)
takuto.ikutaUnsubmitted
Done
ReplyInline Actions

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.

takuto.ikuta: Can I ask you to use option(LLVM_ENABLE_PDB ON) instead? I think LLVM_ENABLE_PDB in if…
rnkAuthorUnsubmitted
Not Done
ReplyInline Actions

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'd switch the condition order, so we check if we're enabling PDBs first, then look at the…
append("/Zi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS) 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)
takuto.ikutaUnsubmitted
Not Done
ReplyInline Actions

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.

takuto.ikuta: Can I ask you to change /OPT:NOICF here also, recommended by docs? https://docs.microsoft.
rongjiecomputerUnsubmitted
Not Done
ReplyInline Actions

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

rongjiecomputer: But won't that penalize everyone with bigger binary size (due to less folding) if they do not…
takuto.ikutaUnsubmitted
Not Done
ReplyInline Actions

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

takuto.ikuta: I see. If ICF is problem on profiled result, I'll disable it in my local or introduce another…
endif (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE") endif()
# /Zc:strictStrings is incompatible with VS12's (Visual Studio 2013's) # /Zc:strictStrings is incompatible with VS12's (Visual Studio 2013's)
# debug mode headers. Instead of only enabling them in VS2013's debug mode, # debug mode headers. Instead of only enabling them in VS2013's debug mode,
Context not available.