This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Support building builtins for a single target
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

phosek updated this revision to Diff 77920.Nov 14 2016, 4:53 PM
phosek retitled this revision from to [compiler-rt] Support building builtins for a single target.
phosek updated this object.
phosek added a reviewer: beanz.
phosek set the repository for this revision to rL LLVM.
phosek added a subscriber: llvm-commits.
beanz edited edge metadata.Nov 15 2016, 10:00 AM

I have one long comment below, but otherwise this is great. Thanks for working on this!

cmake/Modules/CompilerRTUtils.cmake
232 ↗(On Diff #77920)

This is actually bad because of the way CMake's caching behavior works. CMAKE_C_COMPILER_TARGET if set would be a cached variable, and defining another cached variable off of it means that if you update one CMAKE_C_COMPILER_TARGET COMPILER_RT_DEFAULT_TARGET_TRIPLE won't be updated as well.

I'm not sure if there is a good way to change this without breaking existing users. If you can't come up with something can you add a check to ensure that if CMAKE_C_COMPILER_TARGET is defined, it is the same as COMPILER_RT_DEFAULT_TARGET_TRIPLE? That will at least tell users when something goes wrong.

An alternative approach that appeals to me would be to have the code paths based on COMPILER_RT_DEFAULT_TARGET_ONLY use CMAKE_C_COMPILER_TARGET instead of COMPILER_RT_DEFAULT_TARGET_TRIPLE, and make an error if COMPILER_RT_DEFAULT_TARGET_TRIPLE is specified without CMAKE_C_COMPILER_TARGET.

The reason I like that approach is that as users migrate onto COMPILER_RT_DEFAULT_TARGET_ONLY we will be encouraging them to also migrate to using CMake's builtin cross-targeting mechanisms instead of the hand rolled ones.

phosek updated this revision to Diff 78296.Nov 16 2016, 5:20 PM
phosek edited edge metadata.
phosek marked an inline comment as done.
phosek added inline comments.
cmake/Modules/CompilerRTUtils.cmake
249 ↗(On Diff #78296)

I'm not a big fan of the triple parsing business done in CMake, we should probably use the triple directly everywhere without interpreting its content, but that's something that should be done separately as it'll likely require multiple changes.

Ping, could you please take a look again if this solution is fine with you?

beanz accepted this revision.Nov 28 2016, 2:59 PM
beanz edited edge metadata.

Yep, LGTM!

Sorry for the delay, I was out all last week.

This revision is now accepted and ready to land.Nov 28 2016, 2:59 PM
This revision was automatically updated to reflect the committed changes.