Page MenuHomePhabricator

[CMake] Cache the compiler-rt library search results
AcceptedPublic

Authored by phosek on Sep 28 2020, 5:41 PM.

Details

Reviewers
beanz
smeenai
ldionne
steven_wu
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

There's a lot of duplicated calls to find various compiler-rt libraries
from build of runtime libraries like libunwind, libc++, libc++abi and
compiler-rt. The compiler-rt helper module already implemented caching
for results avoid repeated Clang invocations.

This change moves the compiler-rt implementation into a shared location
and reuses it from other runtimes to reduce duplication and speed up
the build.

Diff Detail

Event Timeline

phosek created this revision.Sep 28 2020, 5:41 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptSep 28 2020, 5:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, Restricted Project, dexonsmith and 2 others. · View Herald Transcript
phosek requested review of this revision.Sep 28 2020, 5:41 PM
phosek retitled this revision from [CMake] Cache compiler-rt builtin results to [CMake] Cache compiler-rt library results.

If we require LLVM for any of the projects, perhaps we should hoist the HandleCompilerRT.cmake into LLVM and avoid having to maintain the copy everywhere.

cmake/Modules/HandleCompilerRT.cmake
6

Why change this to a macro? Its setting a cached variable, which should be possible from within a function right?

libcxx/cmake/Modules/HandleCompilerRT.cmake
0

Please make this a function as well.

0

Where is target supposed to be defined? This is tautologically if(CMAKE_CXX_COMPILER_TARGET) no?

14

I think that this deserves a comment explaining what is going on. Using clang's runtime with gcc is possible.

libcxxabi/cmake/Modules/HandleCompilerRT.cmake
0

Please make this a function

libunwind/cmake/Modules/HandleCompilerRT.cmake
0

Same

phosek updated this revision to Diff 295150.Sep 29 2020, 4:34 PM
phosek marked 6 inline comments as done.
phosek added inline comments.
cmake/Modules/HandleCompilerRT.cmake
6

This is an implementation detail which shouldn't be used from outside of this file, I was originally planning on moving it inside find_compiler_rt_library but then I realized we may also want to use it from find_compiler_rt_dir. Maybe we should rename this function to __cache_compiler_rt_library to signal the fact that it's internal?

If we require LLVM for any of the projects, perhaps we should hoist the HandleCompilerRT.cmake into LLVM and avoid having to maintain the copy everywhere.

That's the eventual plan but we haven't yet figured out how the sharing is going to work.

compnerd added inline comments.Sep 30 2020, 8:32 AM
libunwind/cmake/Modules/HandleCompilerRT.cmake
0

I'm still not connecting where target is set.

ldionne requested changes to this revision.Sep 30 2020, 10:42 AM

All those projects should require the whole LLVM monorepo. Let's have a single copy of HandleCompilerRT.

This revision now requires changes to proceed.Sep 30 2020, 10:42 AM

If we require LLVM for any of the projects, perhaps we should hoist the HandleCompilerRT.cmake into LLVM and avoid having to maintain the copy everywhere.

That's the eventual plan but we haven't yet figured out how the sharing is going to work.

What do you mean? Just include(...) the file from the various projects?

If we require LLVM for any of the projects, perhaps we should hoist the HandleCompilerRT.cmake into LLVM and avoid having to maintain the copy everywhere.

That's the eventual plan but we haven't yet figured out how the sharing is going to work.

What do you mean? Just include(...) the file from the various projects?

When migrating to monorepo, we discussed a plan to provide slices for individual projects. While these haven't been provided officially, we already have these for our own purposes (e.g. https://llvm.googlesource.com/llvm-project/libcxx/) and they are used by projects like Chromium. A potential solution would be extend the machinery so that the content of <llvm-project>/cmake/Modules is copied into each slice?

If we require LLVM for any of the projects, perhaps we should hoist the HandleCompilerRT.cmake into LLVM and avoid having to maintain the copy everywhere.

That's the eventual plan but we haven't yet figured out how the sharing is going to work.

What do you mean? Just include(...) the file from the various projects?

When migrating to monorepo, we discussed a plan to provide slices for individual projects. While these haven't been provided officially, we already have these for our own purposes (e.g. https://llvm.googlesource.com/llvm-project/libcxx/) and they are used by projects like Chromium. A potential solution would be extend the machinery so that the content of <llvm-project>/cmake/Modules is copied into each slice?

I wasn't aware of that, and libc++ / libc++abi don't support that. See http://lists.llvm.org/pipermail/libcxx-dev/2020-March/000714.html for example.

In libc++ and libc++abi, we make the assumption that we're checked out at the root of the monorepo. Please sell me the need to support something else, because the only benefit I can think of is "it's faster to clone just libc++", which is not a strong argument to justify such code duplication.

phosek updated this revision to Diff 363162.Jul 30 2021, 11:30 AM
phosek retitled this revision from [CMake] Cache compiler-rt library results to [CMake] Cache the compiler-rt library search results.
phosek edited the summary of this revision. (Show Details)

@ldionne I'm picking up this change again as I'm planning to do a larger cleanup of CMake checks in the coming weeks. I have extracted the compiler-rt logic into a shared module and reused it from other runtimes, let me know if this looks good to you.

phosek updated this revision to Diff 363172.Jul 30 2021, 11:52 AM

@beanz @smeenai Can you take a look as well?

ldionne accepted this revision.Mon, Sep 20, 2:18 PM
ldionne added a subscriber: thakis.

This LGTM, but it's going to break some people who copy only some subdirectories of llvm. I think it's unavoidable, but we'll want to let them know. I think it might apply to e.g. @thakis.

I assume we'll also want to move more common stuff to <root>/cmake in the future? A common place for CMake modules has been sorely needed for a while, thanks for getting the ball rolling!

compiler-rt/cmake/config-ix.cmake
19

Wasn't aware of this, it's pretty awesome.

This LGTM, but it's going to break some people who copy only some subdirectories of llvm. I think it's unavoidable, but we'll want to let them know. I think it might apply to e.g. @thakis.

Chromium is building libc++, libc++abi and libunwind outside of the monorepo, but they use their own GN build so this shouldn't affect them.

I assume we'll also want to move more common stuff to <root>/cmake in the future? A common place for CMake modules has been sorely needed for a while, thanks for getting the ball rolling!

Yes, that's the plan. In particular, after D110005 lands, I'd like to move most of the helper macros and functions from libcxx/cmake/Modules/HandleLibcxxFlags.cmake that are shared across runtimes to <root>/cmake so we don't need to keep them in sync manually.

steven_wu accepted this revision.Mon, Sep 20, 3:38 PM
steven_wu added a subscriber: steven_wu.

LGTM.

I want to do something like this for a long time. It would be good to ReleaseNote the change so people who is distributing/building with a subset of the project directories can be updated to build with top level cmake directory (@ldionne that probably include our internal build system). Some day we might be able to cmake configure with the top level llvm-project directory.

This revision is now accepted and ready to land.Mon, Sep 20, 3:38 PM

This LGTM, but it's going to break some people who copy only some subdirectories of llvm. I think it's unavoidable, but we'll want to let them know. I think it might apply to e.g. @thakis.

AFAIK @tstellar builds the runtimes this way too.

If all the cmake files where installed to <root>/cmake. Distros like Fedora that build LLVM in standalone mode would likely package all these files into a llvm-cmake package and have the other projects depend on it. I actually think doing this will make standalone builds easier. If we do move to this approach, can we make sure the other projects that depend on them don't hardcode source paths, but have the option to include them from any directory on the system. I don' t think supporting this will be too invasive.

smeenai accepted this revision.Tue, Sep 21, 2:14 PM

This looks great, and having a top-level CMake modules directory makes a lot of sense.

There's no LICENSE.TXT file at the top level so there isn't one that clearly applies to the new cmake/. Should compiler-rt/LICENSE.TXT be copied to cmake/LICENSE.TXT?

beanz added a comment.Tue, Sep 21, 3:42 PM

There's no LICENSE.TXT file at the top level so there isn't one that clearly applies to the new cmake/. Should compiler-rt/LICENSE.TXT be copied to cmake/LICENSE.TXT?

Is there any reason not to just put one at the top now?

I assume we aren't going to add a new per-project mirror for the cmake folder, so we might as well just have a license file at the root and not inside the cmake directory. Since all the licenses are the same my understanding was the only reason we kept them per-directory was to ensure they appeared in the mirrors.

There's no LICENSE.TXT file at the top level so there isn't one that clearly applies to the new cmake/. Should compiler-rt/LICENSE.TXT be copied to cmake/LICENSE.TXT?

Is there any reason not to just put one at the top now?

This was suggested before and there wasn't consensus for change. See https://reviews.llvm.org/D64875.