Page MenuHomePhabricator

[CMake] Split the target side of runtimes build
ClosedPublic

Authored by phosek on Dec 16 2020, 10:38 AM.

Details

Reviewers
ldionne
smeenai
beanz
jdoerfert
Group Reviewers
Restricted Project
Restricted Project
Commits
rGb688c5875d08: [CMake] Split the target side of runtimes build
Summary

Previously, llvm/runtimes/CMakeLists.txt played two different roles:

  1. host side which could used to set up the build of runtimes for different targets in the right order;
  2. target side to build the runtimes for the specified target.

This change splits llvm/runtimes/CMakeLists.txt and moves the target
side to runtimes/CMakeLists laying down the foundation for the "A vision
for building the runtimes" proposal. From the user perspective, there
shouldn't be any visible difference at the moment.

Diff Detail

Event Timeline

phosek created this revision.Dec 16 2020, 10:38 AM
phosek requested review of this revision.Dec 16 2020, 10:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 10:38 AM
smeenai accepted this revision.Dec 16 2020, 9:39 PM

LGTM.

I'm a huge fan of this. I had to always spend time figuring out if the code I was reading in the old file was for the host or the target build; having them separated makes things much more understandable IMO.

llvm/runtimes/CMakeLists.txt
1–10

Nit: "a Clang toolchain" or "the Clang toolchain" sounds more gramatically correct to me.

This revision is now accepted and ready to land.Dec 16 2020, 9:39 PM
phosek updated this revision to Diff 315997.Jan 11 2021, 10:30 PM
phosek updated this revision to Diff 315998.Jan 11 2021, 10:31 PM
phosek updated this revision to Diff 315999.
phosek marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.

Oh, I missed this review, but thanks a lot for making this change!!

kaz7 added a subscriber: kaz7.Sat, Nov 20, 3:42 AM

I am looking at codes around this and have noticed this glitch. Others like $(name)_extra_args are initialized with set(${name}_extra_args ${ARG_CMAKE_ARGS}).

llvm/runtimes/CMakeLists.txt
102

Don't we need set(${target}_extra_args ${ARG_CMAKE_ARGS}) here to initialize $(target)_extra_args?

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSat, Nov 20, 3:42 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript