[build-script] Add option to build lld as part of LLVM #41314
Conversation
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test macOS |
|
Yes, windows builds lld already. |
| @@ -429,6 +429,11 @@ def convert_to_impl_arguments(self): | |||
| "--llvm-install-components=%s" % args.llvm_install_components | |||
| ] | |||
|
|
|||
| if args.build_lld: | |||
| impl_args += [ | |||
| "--llvm-install-components=%s" % args.llvm_install_components | |||
There was a problem hiding this comment.
Does llvm install components accumulate in build-script like this? Doesn't this break the usage of llvm-install-components above.
There was a problem hiding this comment.
Not sure how I managed to commit this, but this should have been
if args.build_lld:
impl_args += ["--build-lld"]
🤦
| @@ -429,6 +429,11 @@ def convert_to_impl_arguments(self): | |||
| "--llvm-install-components=%s" % args.llvm_install_components | |||
utils/build-script-impl
Outdated
| @@ -1832,7 +1833,7 @@ for host in "${ALL_HOSTS[@]}"; do | |||
| # | |||
| # This makes it easier to build target stdlibs on systems that | |||
| # have old toolchains without more modern linker features. | |||
| if [[ "$(uname -s)" != "Darwin" ]] ; then | |||
| if [[ "$(uname -s)" != "Darwin" || "${BUILD_LLD}" ]] ; then | |||
| if [[ ! "${SKIP_BUILD_LLD}" ]]; then | |||
There was a problem hiding this comment.
@ahoppen I was thinking... we already have a skip-build-lld option... so we really shouldn't add another build-lld option to build-script-impl. In general we want to simplify build-script-impl by not adding new state like this.
I have a counter-proposal, why don't we remove the Darwin check here. Instead, in build-script lets just pass in skip-build-lldb to build-script-impl if we are on Darwin and build-lld isn't set. Then build-script-impl is simpler and doesn't have an additional variable. What are your thoughts?
ahoppen
force-pushed
the
pr/build-lld
branch
from
11e84ae to
a711a66
Compare
|
Very good idea, @gottesmm. I changed the PR to remove the Darwin check in build-script-impl and instead make build-script decide whether lld should be built. |
|
@swift-ci Please smoke test |
| llvm_enable_projects+=("lld") | ||
| fi | ||
| if [[ ! "${SKIP_BUILD_LLD}" ]]; then | ||
| llvm_enable_projects+=("lld") |
There was a problem hiding this comment.
Can you add a build-script unit test that makes sure we don't break this by mistake? Then I think this LGTM. I am also fine if you want to merge this now and write the test in a subsequent PR.
There was a problem hiding this comment.
What do you mean by a build-script unit test? I don’t think I ever saw unit tests for build-script 🤔
There was a problem hiding this comment.
I added a unit test. @gottesmm Could you take another look?
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux |
|
@swift-ci Please smoke test |
I find that lld links faster than the default linker for release builds and it’s convenient if we can just build it as part of Swift’s LLVM build.
Note that to use lld, you still need to set
extra-cmake-options=-DLLVM_ENABLE_LLD:BOOL=TRUEmanually. This PR does not do so because we can only start using that flag after lld has been built.