Allow disabling PDB generation in Release build
ClosedPublic

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

Details

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

Repository
rL LLVM
rongjiecomputer edited the summary of this revision. (Show Details)
takuto.ikuta added inline comments.Sat, Feb 10, 3:22 PM
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 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)Sat, Feb 10, 4:09 PM
takuto.ikuta added inline comments.Sat, Feb 10, 4:41 PM
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.

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 accepted this revision.Sat, Feb 10, 6:51 PM

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.

This revision is now accepted and ready to land.Sat, Feb 10, 6:51 PM

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

Nico, can I ask you to land this?

rnk added a comment.Tue, Feb 13, 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.

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

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

This revision now requires changes to proceed.Thu, Feb 15, 11:14 AM
rnk commandeered this revision.Thu, Feb 15, 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.Thu, Feb 15, 11:15 AM
This revision was automatically updated to reflect the committed changes.