Page MenuHomePhabricator

[llvm/runtimes] Add runtimes as a dependency of clang-bootstrap-deps
ClosedPublic

Authored by xinxinw1 on Dec 12 2019, 11:50 AM.

Details

Summary

With the new LLVM_ENABLE_RUNTIMES option introduced in https://reviews.llvm.org/D40233, compiler-rt can now be included as a runtime. Since compiler-rt is needed for PGO, runtimes needs to be included as a dependency of clang-bootstrap-deps when building the stage1 compiler.

Diff Detail

Event Timeline

xinxinw1 created this revision.Dec 12 2019, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 11:50 AM
smeenai added subscribers: phosek, beanz, smeenai.

This makes sense to me ... @beanz @phosek what do you think?

llvm/runtimes/CMakeLists.txt
566

I'd say "we need the profile runtime" instead of "we need it", just to be a bit more specific.

phosek added inline comments.Dec 14 2019, 7:46 AM
llvm/runtimes/CMakeLists.txt
567

I don't think we can add this dependency unconditionally. If LLVM_ENABLE_RUNTIMES isn't set, runtimes list would end up empty here https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt#L32, in which case we wouldn't define runtimes target at all here https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt#L497.

We also cannot just check whether LLVM_ENABLE_RUNTIMES is set because in theory we still support the nested layout where users can place runtimes directly into llvm/runtimes subdirectory; we probably don't need this now that we're in monorepo, but we shouldn't break it until we officially announce the deprecation of that feature.

xinxinw1 marked an inline comment as done.Dec 16 2019, 10:42 AM
xinxinw1 added inline comments.
llvm/runtimes/CMakeLists.txt
567

We're not adding this dependency unconditionally. This line is under an if(runtimes) block, so at this point where we're adding runtimes to clang-bootstrap-deps, the runtimes list won't be empty and the runtimes target would be defined. Additionally, if the runtimes target wasn't defined, then the runtimes-configure target which is referenced in the line above also wouldn't be defined, but since that line works fine, adding an additional dependency on runtimes should be fine.

xinxinw1 marked an inline comment as not done.Dec 16 2019, 10:42 AM
xinxinw1 updated this revision to Diff 234105.Dec 16 2019, 10:47 AM

Update comment text

xinxinw1 marked an inline comment as done.Dec 16 2019, 10:47 AM
phosek accepted this revision.Dec 16 2019, 12:29 PM

LGTM

This revision is now accepted and ready to land.Dec 16 2019, 12:29 PM
This revision was automatically updated to reflect the committed changes.