Page MenuHomePhabricator

[compiler-rt] Enable building builtins using top-level CMake file
Needs ReviewPublic

Authored by phosek on Oct 15 2020, 12:16 PM.

Details

Summary

When building builtins separately as part of the runtime build, we
also want to include crt. To enable that, enable building builtins
together with crt using the top-level CMake file with a new option
COMPILER_RT_BOOTSTRAP set.

Diff Detail

Event Timeline

phosek created this revision.Oct 15 2020, 12:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 15 2020, 12:16 PM
Herald added subscribers: llvm-commits, Restricted Project, s.egerton and 3 others. · View Herald Transcript
phosek requested review of this revision.Oct 15 2020, 12:16 PM

This is an alternative to D70744. I'm not a fan of introducing compiler-rt/builtins as a new top-level CMake, so I was trying to come up with an alternative and this is what I came up with, let me know what you think.

This is an alternative to D70744. I'm not a fan of introducing compiler-rt/builtins as a new top-level CMake, so I was trying to come up with an alternative and this is what I came up with, let me know what you think.

I think this is an improvement, it was awkward to have two top-level entry points to the cmake anyway. This probably allows for some further de-duplication down the road as well.

compiler-rt/cmake/builtin-config-ix.cmake
61
210

It would be nice to print the list of supported crt arch here as well.

211

OS_NAME doesn't currently get set in builtin-config-ix, we'll need to duplicate the setup from config-ix.

compiler-rt/lib/builtins/CMakeLists.txt
8

We probably should retain the new cmake minimum

llvm/runtimes/CMakeLists.txt
269–270

We should probably disable the crt build here now.

347–348

Ditto the other comment

phosek updated this revision to Diff 298559.Oct 16 2020, 12:22 AM
phosek marked 6 inline comments as done.
daltenty accepted this revision.Oct 19 2020, 3:11 PM

LGTM, with slight adjustment. Though I will note that this requires a clean build because CMake will complain about the change in source directory in cache:

CMake Error: The source “../compiler-rt/CMakeLists.txt" does not match the source ../compiler-rt/lib/builtins/CMakeLists.txt" used to generate cache. Re-run cmake with a different source directory.

compiler-rt/CMakeLists.txt
124

fyi, this needs a slight adjustments because D87120 has landed

This revision is now accepted and ready to land.Oct 19 2020, 3:11 PM
daltenty requested changes to this revision.Oct 23 2020, 7:43 AM
daltenty added inline comments.
compiler-rt/cmake/builtin-config-ix.cmake
212

The problem we have here is that the scope for these variables has changed, builtin-config-ix.cmake is only included in the builtins CMakeList.tx, so when we go to check COMPILER_RT_HAS_CRT it's out of scope.

Perhaps it would be cleaner to split the CRT checks into it's own crt-config-ix.cmake that can be included at the appropriate scope?

This revision now requires changes to proceed.Oct 23 2020, 7:43 AM
phosek updated this revision to Diff 330160.Mar 11 2021, 11:58 PM
phosek marked 2 inline comments as done.
phosek edited the summary of this revision. (Show Details)
phosek updated this revision to Diff 330162.Mar 12 2021, 12:04 AM