[PATCH] D62111: Tweaks for setting CMAKE_LINKER to lld-link

Wed May 22 05:29:16 PDT 2019

thakis marked an inline comment as done.
thakis added inline comments.

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:15

-if(CMAKE_LINKER MATCHES "lld-link\\.exe" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld") OR LLVM_ENABLE_LLD)
+if(CMAKE_LINKER MATCHES "lld-link" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld") OR LLVM_ENABLE_LLD)
   set(LINKER_IS_LLD_LINK TRUE)
----------------
smeenai wrote:
> Not your diff, but is this just setting `LINKER_IS_LLD_LINK` to true when `LLVM_ENABLE_LLD` is true? That seems wrong?
You're saying that this should be `OR (WIN32 AND (LLVM_USE_LINKER STREQUAL "lld" OR LLVM_ENABLE_LLD))`, right?

I think this whole conditional is a bit shaky:

- Since ` LLVM_ENABLE_LLD` causes `LLVM_USE_LINKER` to be set, we should probably only look at the latter

- On Windows, as far as I know cmake always calls (lld-)link.exe (it certainly does with the Ninja generator, which is what I use), so I think this should really only check `CMAKE_LINKER` (…which by default is ignored on non-Windows).

But after this change, `LINKER_IS_LLD_LINK` is only used in lto-related contexts, and I'm guessing nobody uses lto to build llvm on windows yet, so it probably doesn't matter all that much.

We're planning to look at thinlto for chromium's llvm builds "soon", so we might revisit this then.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62111/new/

https://reviews.llvm.org/D62111