This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Create install targets for Darwin libraries
ClosedPublic

Authored by smeenai on May 3 2019, 3:15 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai created this revision.May 3 2019, 3:15 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 3 2019, 3:15 PM
Herald added subscribers: Restricted Project, mgorny, dberris. · View Herald Transcript
phosek accepted this revision.May 6 2019, 3:50 PM

LGTM

compiler-rt/cmake/Modules/CompilerRTUtils.cmake
423 ↗(On Diff #198103)

IIUC, only the first invocation of this function will create the parent target and all the following ones will reuse it (for all systems/architectures with the same parent target that is). One possible alternative would be to split this into its function to avoid all those extra checks on subsequent invocations of this function.

This revision is now accepted and ready to land.May 6 2019, 3:50 PM
smeenai marked an inline comment as done.May 6 2019, 3:57 PM
smeenai added inline comments.
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
423 ↗(On Diff #198103)

Hmm, so the split function would just create the install-$parent targets? How would we ensure the split function gets called for each $parent without doing some sort of existence check?

Note that I've copied this logic from similar logic in AddCompilerRT.cmake: https://github.com/llvm/llvm-project/blob/491746a584723c147f8910c1ad5051c507afd735/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L237

Will address any additional comments post-commit. Thanks for the quick review!

smeenai marked an inline comment as done.May 7 2019, 11:57 AM
smeenai added inline comments.
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
423 ↗(On Diff #198103)

Ah, I guess you meant having a second function that's called outside the loop in AddCompilerRT? I'm not sure if target checks are expensive enough to make that worth it ... I couldn't measure any non-noise CMake configure time difference before and after this patch.

This revision was automatically updated to reflect the committed changes.