Page MenuHomePhabricator

[CMake] Support lld with LTO bootstrap
ClosedPublic

Authored by phosek on Nov 14 2016, 4:44 PM.

Details

Summary

lld has LTO support, if requested we should add a dependency on lld rather than LLVMgold when doing LTO bootstrap build.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek updated this revision to Diff 77916.Nov 14 2016, 4:44 PM
phosek retitled this revision from to [CMake] Support lld with LTO bootstrap.
phosek updated this object.
phosek added a reviewer: beanz.
phosek set the repository for this revision to rL LLVM.
phosek added subscribers: cfe-commits, ruiu.
ruiu added a reviewer: pcc.Nov 14 2016, 4:46 PM

Make sense to me, but I don't think it is enough: LLVM_ENABLE_LLD should be passed to stage-2, and it also should be set to the absolute path of the stage-1 lld build.

CMakeLists.txt
516 ↗(On Diff #77916)

This dep does not make sense when using either lld or gold by the way.

Make sense to me, but I don't think it is enough: LLVM_ENABLE_LLD should be passed to stage-2, and it also should be set to the absolute path of the stage-1 lld build.

What is search directories order in Clang? It's reasonable it it tries to look into same directory first. In this case CMAKE_C_COMPILER/CMAKE_CXX_COMPILER should be enough for later stages.

It's sufficient, I just tested it. I'm not actually sure if LLVM_ENABLE_LLD here is correct, LLVM_ENABLE_LLD forces the use of lld, but lld might not be available during first stage. We need LLVM_ENABLE_LLD to be set, but only for the second stage (which is something that can be done in the cache file), in the first stage we should probably just check whether lld source is available and we're building it, but I'm not sure what's the easiest/cleanest way to do that?

It's sufficient, I just tested it. I'm not actually sure if LLVM_ENABLE_LLD here is correct, LLVM_ENABLE_LLD forces the use of lld, but lld might not be available during first stage. We need LLVM_ENABLE_LLD to be set, but only for the second stage (which is something that can be done in the cache file), in the first stage we should probably just check whether lld source is available and we're building it, but I'm not sure what's the easiest/cleanest way to do that?

-DBOOTSTRAP_ LLVM_ENABLE_LLD is the right way to check for stage-2 options.

If you look just a few lines above your check you'll see an example of check for BOOTSTRAP_LLVM_ENABLE_LTO

I think this should be handled in higher level script (utils/release/test-release.sh or similar), not in CMake. CMake compiler tests just need to fail when LLVM_ENABLE_LLD is used without actually having them.

I think this should be handled in higher level script (utils/release/test-release.sh or similar), not in CMake. CMake compiler tests just need to fail when LLVM_ENABLE_LLD is used without actually having them.

I don't understand what you mean here, mind elaborating a bit?

I meant that multi-stage build is processed by higher level script. So it should take care about consistency of source code srt versus later stages build options.

I meant that multi-stage build is processed by higher level script. So it should take care about consistency of source code srt versus later stages build options.

We have a 2-stage build directly with a single CMake invocation, this is what this code path is about.
Are you referring to this situation or are you referring to 2-stage builds that are not "visible" from CMake itself and requires to first build stage-1 before configuring stage-2 with a second cmake invocation? (If I understand correctly I suspect you meant the latter)

Yes, you are correct, I meant later.

It's sufficient, I just tested it.

How did you check it? I don't understand how LLVM_ENABLE_LLD is propagated to stage-2?

CMakeLists.txt
534 ↗(On Diff #77916)

What if not by the way? Should we error out here? What is the expected behavior?

It's sufficient, I just tested it.

How did you check it? I don't understand how LLVM_ENABLE_LLD is propagated to stage-2?

Sufficient as in Clang looks for lld in the same directory where clang/clang++ binary is first so we don't need to explicitly pass the path to lld to later stages.

CMakeLists.txt
534 ↗(On Diff #77916)

I assume that's platform dependent; if some platforms (other than Darwin) use linker other than gold or lld for LTO build, erroring out here might break them. I don't think there are any such platforms though because they'd be broken already as we currently always try to use LLVMgold which causes CMake error in case we're not building with Binutils (that's how I discovered this issue, since we're using lld rather than gold on our platform).

It's sufficient, I just tested it.

How did you check it? I don't understand how LLVM_ENABLE_LLD is propagated to stage-2?

Sufficient as in Clang looks for lld in the same directory where clang/clang++ binary is first so we don't need to explicitly pass the path to lld to later stages.

OK, but still, LLVM_ENABLE_LLD needs to be passed to stage-2, so it needs to be actually BOOTSTRAP_LLVM_ENABLE_LLD.
I looked at all the CMake _PASSTHROUGH and didn't find it mentioned anywhere. We could make it auto-forwarded in this case maybe, @beanz is best to answer this.

Have you looked into turning if(LLVM_ENABLE_LLD) into if(BOOTSTRAP_LLVM_ENABLE_LLD)?
Technically we may want to have the stage-2 linked with lld even if lld is not on the system and not available during stage1.

phosek updated this revision to Diff 77938.Nov 14 2016, 7:00 PM
phosek marked an inline comment as done.

OK, but still, LLVM_ENABLE_LLD needs to be passed to stage-2, so it needs to be actually BOOTSTRAP_LLVM_ENABLE_LLD.
I looked at all the CMake _PASSTHROUGH and didn't find it mentioned anywhere. We could make it auto-forwarded in this case maybe, @beanz is best to answer this.

Have you looked into turning if(LLVM_ENABLE_LLD) into if(BOOTSTRAP_LLVM_ENABLE_LLD)?
Technically we may want to have the stage-2 linked with lld even if lld is not on the system and not available during stage1.

Yes, I just finished a test build and BOOTSTRAP_LLVM_ENABLE_LLD seems to work, see the updated diff.

mehdi_amini accepted this revision.Nov 14 2016, 8:40 PM
mehdi_amini added a reviewer: mehdi_amini.

LGTM.

This revision is now accepted and ready to land.Nov 14 2016, 8:40 PM
mehdi_amini requested changes to this revision.Nov 14 2016, 8:41 PM
mehdi_amini edited edge metadata.
mehdi_amini added inline comments.
CMakeLists.txt
516 ↗(On Diff #77938)

This is needed in the "if(APPLE)" case.
And conditonalized by "if(!BOOTSTRAP_LLVM_ENABLE_LLD))"

530 ↗(On Diff #77938)

Actually, not clear why this is behind if(!APPLE)

This revision now requires changes to proceed.Nov 14 2016, 8:41 PM

What about the following logic?

 if(BOOTSTRAP_LLVM_ENABLE_LLD)
      add_dependencies(clang-bootstrap-deps lld)
 endif()
 if(APPLE)
    # on Darwin we need to set DARWIN_LTO_LIBRARY so that -flto will work
    # using the just-built compiler, and we need to override DYLD_LIBRARY_PATH
    # so that the host object file tools will use the just-built libLTO.
    # However if System Integrity Protection is enabled the DYLD variables
    # will be scrubbed from the environment of any base system commands. This
    # includes /bin/sh, which ninja uses when executing build commands. To
    # work around the envar being filtered away we pass it in as a CMake
    # variable, and have LLVM's CMake append the envar to the archiver calls.
    set(LTO_LIBRARY -DDARWIN_LTO_LIBRARY=${LLVM_SHLIB_OUTPUT_INTDIR}/libLTO.dylib
      -DDYLD_LIBRARY_PATH=${LLVM_LIBRARY_OUTPUT_INTDIR})
  elseif(NOT WIN32)
    add_dependencies(clang-bootstrap-deps llvm-ar llvm-ranlib)
    if(NOT LLVM_ENABLE_LLD AND LLVM_BINUTILS_INCDIR)
      add_dependencies(clang-bootstrap-deps LLVMgold)
    endif()
    set(LTO_AR -DCMAKE_AR=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-ar)
    set(LTO_RANLIB -DCMAKE_RANLIB=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-ranlib)
  endif()
endif()

I'm not even sure why we don't use llvm-ar and llvm-ranlib on OSX by the way, this makes the logic more complicated here, but there might be a good reason for that.

AFAIK lld on Darwin doesn't support LTO (last I heard it cannot even self-host at the moment) so I'm not sure if it makes sense to enable it if(APPLE).

AFAIK lld on Darwin doesn't support LTO (last I heard it cannot even self-host at the moment) so I'm not sure if it makes sense to enable it if(APPLE).

OK, we could error instead of ignoring then.
But you still need to restore the dependency on libLTO, unless I'm missing something.

phosek updated this revision to Diff 77950.Nov 14 2016, 11:19 PM
phosek edited edge metadata.
beanz edited edge metadata.Nov 15 2016, 10:20 AM

On Darwin you don't want to set DARWIN_LTO_LIBRARY on the next stage if you're intending the next stage to use lld because that sets flags that aren't supported by lld.

On Darwin you don't want to set DARWIN_LTO_LIBRARY on the next stage if you're intending the next stage to use lld because that sets flags that aren't supported by lld.

Right, but that's a separate issue than what is addressed here right?
The issue already exists: setting both BOOTSTRAP_LLVM_ENABLE_LTO and LLVM_ENABLE_LLD can't work on Darwin right now.

mehdi_amini accepted this revision.Nov 15 2016, 10:22 AM
mehdi_amini edited edge metadata.

Patch LGTM right now, let's just wait for @beanz in case he objects.

This revision is now accepted and ready to land.Nov 15 2016, 10:22 AM
beanz accepted this revision.Nov 15 2016, 11:30 AM
beanz edited edge metadata.

Yep. Can we maybe add an error to that effect? I really don't want the build system ignoring invalid configurations.

Otherwise LGTM!

phosek updated this revision to Diff 78045.Nov 15 2016, 11:44 AM
phosek edited edge metadata.

Added error message for Darwin, does this look good to you?

@beanz : don't you think that it should be handled at the top level and not in the bootstrap logic?
Right now we don't error when invoking cmake with -DLLVM_ENABLE_LLD=ON and -DLLVM_ENABLE_LTO=ON on Darwin.

@beanz : don't you think that it should be handled at the top level and not in the bootstrap logic?
Right now we don't error when invoking cmake with -DLLVM_ENABLE_LLD=ON and -DLLVM_ENABLE_LTO=ON on Darwin.

That might a better approach, I tried invoking -DLLVM_ENABLE_LLD=ON on Darwin and it fails, even without LLVM_ENABLE_LTO; the CXX_SUPPORTS_LLD check which checks whether lld is supported by the driver succeeds, but all subsequent ones fail because lld fails.

beanz added a comment.Nov 15 2016, 1:12 PM

Failing on -DLLVM_ENABLE_LLD=ON and -DLLVM_ENABLE_LTO=ON in LLVM seems fine to me.

phosek updated this revision to Diff 78096.Nov 15 2016, 4:31 PM

I've removed the error message and moved it to D26715.

Patch still LGTM!
Thanks.

beanz added a comment.Nov 16 2016, 3:50 PM

Yep, LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jan 14 2017, 6:57 PM
cfe/trunk/CMakeLists.txt
531

I come back to this a bit late, sorry, but I'm not sure I understand why this dependency on the bootstrap happens only when LTO is used?

beanz added inline comments.Jan 17 2017, 10:21 AM
cfe/trunk/CMakeLists.txt
531

We should probably separate LLVM_ENABLE_LTO and LLVM_ENABLE_LLD. You can use LLD without LTO on a wider variety of platforms (including OS X), and you might want to use LLD without LTO.

phosek added inline comments.Jan 17 2017, 12:09 PM
cfe/trunk/CMakeLists.txt
531

Yes, this is unnecessarily restrictive; see D28821.