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

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

710

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

710

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

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.

710

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

Isn't this check redundant now?

720

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

725

Ditto.

[inglorion]

cmake/modules/HandleLLVMOptions.cmake

709

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

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

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 ↗

(On Diff #90213)

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 ↗

(On Diff #90213)

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 Diff #90213)

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 ↗

(On Diff #90213)

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

cmake/modules/HandleLLVMOptions.cmake

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)

 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 CMAKE_LINKER MATCHES "lld")
+  message(FATAL_ERROR "When compiling for Windows, LLVM_ENABLE_LTO requires using lld as the linker (set CMAKE_LINKER to 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 CMAKE_LINKER MATCHES "link.exe$")
+    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 CMAKE_LINKER MATCHES "link.exe$")
+    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 CMAKE_LINKER MATCHES "link.exe$")
+    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)