enable building with LTO on Windows using clang-cl and lld

[inglorion]

Summary

With clang-cl gaining support for link-time optimization, we can now enable builds using LTO when using clang-cl and lld on Windows. To do this, we must not pass the -flto flag to the linker; lld-link does not understand it, but will perform LTO automatically when it encounters bitcode files. We also don't pass /Brepro when using LTO - the compiler doesn't generate object files for LTO, so passing the flag would only result in a warning about it being unused.

[hans]

This is cool! Does the lto/thinlto build actually work, tests passing etc?

cmake/modules/HandleLLVMOptions.cmake

416 ↗

(On Diff #89316)

Maybe we should just make /Brepro be ignored with -flto in clang-cl instead of making users remove the flag?

707 ↗

(On Diff #89316)

Does this mean that if the user sets LLVM_ENABLE_LTO but forgets to set the build to use lld, we silently don't do LTO? Maybe we should error in the non-lld case instead?

[mehdi_amini]

cmake/modules/HandleLLVMOptions.cmake

707 ↗

(On Diff #89316)

Especially since otherwise the link will fail badly right? Preventing from generating an invalid configuration seems better to me.

[inglorion]

cmake/modules/HandleLLVMOptions.cmake

416 ↗

(On Diff #89316)

I considered that, but it's not what clang (non-cl) does. I would be happy to implement that (for both?) if we feel that is the right way to go. For now, I decided to keep clang-cl consistent with clang and change the build configuration to not pass an assembler flag when we're not using the assembler.

707 ↗

(On Diff #89316)

Actually, using LLVM_ENABLE_LTO with MSVC's linker would fail with and without this change. Having said that, I agree it makes sense to think about how we can make this friendlier while we're making changes anyway.

[inglorion]

added error message for when unsupported combination of platform, LTO, and linker is used

[hans]

cmake/modules/HandleLLVMOptions.cmake

709 ↗

(On Diff #89935)

Isn't this check redundant now?

720 ↗

(On Diff #89935)

We checked above that it's not link.exe.
But could the linker be gold and get confused about this flag?

725 ↗

(On Diff #89935)

Ditto.

[inglorion]

cmake/modules/HandleLLVMOptions.cmake

709 ↗

(On Diff #89935)

It shouldn't be. This is used to differentiate lld-link.exe from things like ld and lld, which do need the flag. It also matches link.exe without "lld-", which perhaps is a bit confusing. I can change it to look for lld-link, specifically, if folks feel that would be better.

[hans]

cmake/modules/HandleLLVMOptions.cmake

709 ↗

(On Diff #89935)

Oh right, I was reading this as checking for link.exe, not that it might also be lld-link.exe :-) This is subtle, but I'm not sure how to make it better.

[inglorion]

refactored check for lld-link to be more explicit and less confusing

cmake/modules/HandleLLVMOptions.cmake

709 ↗

(On Diff #89935)

I rewrote this to make the intent clearer, and also used the new way in the other places where we were checking for lld-link.

[hans]

lgtm

[mehdi_amini]

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

717

I don't understand this, this sounds suspicious to me. This is the flag we use to invoke *clang* for the linker step AFAIK, and it is up to clang to not pass the flag down to the lld-link job if not supported.

717

For instance ld64 on macOS does not accept -flto either, but we still configure -flto here for the link stage invocation and leave up to the clang driver to handle the correct linker invocation.

[rnk]

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

717

On Windows, nobody invokes the compiler to do the link step, except people porting build systems from Unix.

[mehdi_amini]

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

717

Oh, OK, didn't know that! Nevermind then :)

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

 # This CMake module is responsible for interpreting the user defined LLVM_
 # options and executing the appropriate CMake commands to realize the users'
 # selections.
 
 # This is commonly needed so make sure it's defined before we include anything
 # else.
 string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
 
 include(CheckCompilerVersion)
 include(HandleLLVMStdlib)
 include(AddLLVMDefinitions)
 include(CheckCCompilerFlag)
 include(CheckCXXCompilerFlag)
 
+if(CMAKE_LINKER MATCHES "lld-link.exe" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld"))
+  set(LINKER_IS_LLD_LINK TRUE)
+else()
+  set(LINKER_IS_LLD_LINK FALSE)
+endif()
+
 # Ninja Job Pool support
 # The following only works with the Ninja generator in CMake >= 3.0.
 set(LLVM_PARALLEL_COMPILE_JOBS "" CACHE STRING
   "Define the maximum number of concurrent compilation jobs.")
 if(LLVM_PARALLEL_COMPILE_JOBS)
   if(NOT CMAKE_MAKE_PROGRAM MATCHES "ninja")
     message(WARNING "Job pooling is only available with Ninja generators.")
   else()
if(NOT CMAKE_MAKE_PROGRAM MATCHES "ninja")
     message(WARNING "Job pooling is only available with Ninja generators.")
   else()
     set_property(GLOBAL APPEND PROPERTY JOB_POOLS link_job_pool=${LLVM_PARALLEL_LINK_JOBS})
     set(CMAKE_JOB_POOL_LINK link_job_pool)
   endif()
 endif()
 
 
-if (CMAKE_LINKER MATCHES "lld-link.exe")
+if (LINKER_IS_LLD_LINK)
   # Pass /MANIFEST:NO so that CMake doesn't run mt.exe on our binaries.  Adding
   # manifests with mt.exe breaks LLD's symbol tables and takes as much time as
   # the link. See PR24476.
   append("/MANIFEST:NO"
     CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
 endif()
 
 if( LLVM_ENABLE_ASSERTIONS )
if( MSVC )
   endif(NOT (MSVC_VERSION LESS 1900))
 
   # "Generate Intrinsic Functions".
   append("/Oi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
   # "Enforce type conversion rules".
   append("/Zc:rvalueCast" CMAKE_CXX_FLAGS)
 
-  if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+  if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT LLVM_ENABLE_LTO)
     # clang-cl and cl by default produce non-deterministic binaries because
     # link.exe /incremental requires a timestamp in the .obj file.  clang-cl
     # has the flag /Brepro to force deterministic binaries. We want to pass that
-    # whenever you're building with clang unless you're passing /incremental.
+    # whenever you're building with clang unless you're passing /incremental
+    # or using LTO (/Brepro with LTO would result in a warning about the flag
+    # being unused, because we're not generating object files).
     # This checks CMAKE_CXX_COMPILER_ID in addition to check_cxx_compiler_flag()
     # because cl.exe does not emit an error on flags it doesn't understand,
     # letting check_cxx_compiler_flag() claim it understands all flags.
     check_cxx_compiler_flag("/Brepro" SUPPORTS_BREPRO)
     if (SUPPORTS_BREPRO)
       # Check if /INCREMENTAL is passed to the linker and complain that it
       # won't work with /Brepro.
       string(TOUPPER "${CMAKE_EXE_LINKER_FLAGS}" upper_exe_flags)
if (NOT MSVC)
     endif()
     # Use -O1 even in debug mode, otherwise sanitizers slowdown is too large.
     if (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
       add_flag_if_supported("-O1" O1)
     endif()
   elseif (CLANG_CL)
     # Keep frame pointers around.
     append("/Oy-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
-    if (CMAKE_LINKER MATCHES "lld-link.exe")
+    if (LINKER_IS_LLD_LINK)
       # Use DWARF debug info with LLD.
       append("-gdwarf" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
     else()
       # Enable codeview otherwise.
       append("/Z7" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
     endif()
     # Always ask the linker to produce symbols with asan.
     append("-debug" CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)

 append_if(LLVM_BUILD_INSTRUMENTED_COVERAGE "-fprofile-instr-generate='${LLVM_PROFILE_FILE_PATTERN}' -fcoverage-mapping"
   CMAKE_CXX_FLAGS
   CMAKE_C_FLAGS
   CMAKE_EXE_LINKER_FLAGS
   CMAKE_SHARED_LINKER_FLAGS)
 
 set(LLVM_ENABLE_LTO OFF CACHE STRING "Build LLVM with LTO. May be specified as Thin or Full to use a particular kind of LTO")
 string(TOUPPER "${LLVM_ENABLE_LTO}" uppercase_LLVM_ENABLE_LTO)
+if(LLVM_ENABLE_LTO AND LLVM_ON_WIN32 AND NOT LINKER_IS_LLD_LINK)
+  message(FATAL_ERROR "When compiling for Windows, LLVM_ENABLE_LTO requires using lld as the linker (point CMAKE_LINKER at lld-link.exe)")
+endif()
 if(uppercase_LLVM_ENABLE_LTO STREQUAL "THIN")
-  append("-flto=thin" CMAKE_CXX_FLAGS CMAKE_C_FLAGS
-                      CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+  append("-flto=thin" CMAKE_CXX_FLAGS CMAKE_C_FLAGS)
+  if(NOT LINKER_IS_LLD_LINK)
+    append("-flto=thin" CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+  endif()
   # On darwin, enable the lto cache. This improves initial build time a little
   # since we re-link a lot of the same objects, and significantly improves
   # incremental build time.
   append_if(APPLE "-Wl,-cache_path_lto,${PROJECT_BINARY_DIR}/lto.cache"
             CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
 elseif(uppercase_LLVM_ENABLE_LTO STREQUAL "FULL")
-  append("-flto=full" CMAKE_CXX_FLAGS CMAKE_C_FLAGS
-                 CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+  append("-flto=full" CMAKE_CXX_FLAGS CMAKE_C_FLAGS)
+  if(NOT LINKER_IS_LLD_LINK)
+    append("-flto=full" CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+  endif()
 elseif(LLVM_ENABLE_LTO)
-  append("-flto" CMAKE_CXX_FLAGS CMAKE_C_FLAGS
-                 CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+  append("-flto" CMAKE_CXX_FLAGS CMAKE_C_FLAGS)
+  if(NOT LINKER_IS_LLD_LINK)
+    append("-flto" CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
+  endif()
 endif()
 
 # This option makes utils/extract_symbols.py be used to determine the list of
 # symbols to export from LLVM tools. This is necessary when using MSVC if you
 # want to allow plugins, though note that the plugin has to explicitly link
 # against (exactly one) tool so we can't unilaterally turn on
 # LLVM_ENABLE_PLUGINS when it's enabled.
 option(LLVM_EXPORT_SYMBOLS_FOR_PLUGINS "Export symbols from LLVM tools so that plugins can import them" OFF)