This is an archive of the discontinued LLVM Phabricator instance.

[gn] Support for building compiler-rt builtins
ClosedPublic

Authored by phosek on Apr 5 2019, 11:52 AM.

Details

Summary

This is support for building compiler-rt builtins, The library build
should be complete for a subset of supported platforms, but not all
CMake options have been replicated in GN.

We always use the just built compiler to build all the runtimes, which
is equivalent to the CMake runtimes build. This simplifies the build
configuration because we don't need to support arbitrary host compiler
and can always assume the latest Clang. With GN's toolchain support,
this is significantly more efficient than the CMake runtimes build.

Diff Detail

Event Timeline

phosek created this revision.Apr 5 2019, 11:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 11:52 AM
phosek updated this revision to Diff 193930.Apr 5 2019, 12:01 PM
phosek edited the summary of this revision. (Show Details)

lgtm except for minor comments:

llvm/utils/gn/secondary/compiler-rt/lib/BUILD.gn
3

llvm/utils/gn/gn.py help label: "Stylistically, we prefer to use absolute paths for all non-file-local references"

llvm/utils/gn/secondary/compiler-rt/lib/builtins/BUILD.gn
30

Edit llvm/utils/gn/build/sync_source_lists_from_cmake.py and modify gn_cpp_re and cmake_cpp_re to contain |c after cpp and make sure the script is still happy. You probably have to move a few closing parens in compiler-rt/lib/builtins/CMakeLists.txt to the next line (which is by far the more common style in LLVM's cmake, so this will make the cmake build more consistent too).

Hm, looking at that file, we might want to add |S there too.

phosek updated this revision to Diff 193943.Apr 5 2019, 12:50 PM
phosek marked 2 inline comments as done.
phosek updated this revision to Diff 193947.Apr 5 2019, 12:55 PM
phosek updated this revision to Diff 193948.Apr 5 2019, 12:58 PM
thakis accepted this revision.Apr 5 2019, 12:58 PM

Thanks! lgtm with the comment for the one one-line sources line as discussed.

compiler-rt/lib/builtins/CMakeLists.txt
154 ↗(On Diff #193943)

(the more common style is to indent the ) by two. unless you're being consistent with other places in this file, consider indenting the closing parens)

llvm/utils/gn/secondary/compiler-rt/lib/builtins/BUILD.gn
366

ppc64 too, or are these 32-bit?

This revision is now accepted and ready to land.Apr 5 2019, 12:58 PM
phosek updated this revision to Diff 193949.Apr 5 2019, 1:01 PM
phosek updated this revision to Diff 193950.
phosek marked an inline comment as done.
phosek added inline comments.Apr 5 2019, 1:03 PM
compiler-rt/lib/builtins/CMakeLists.txt
154 ↗(On Diff #193943)

Other sets in this file already used this style so I was trying to be consistent with those. Do you want me to reformat all of them?

thakis added inline comments.Apr 5 2019, 1:04 PM
compiler-rt/lib/builtins/CMakeLists.txt
154 ↗(On Diff #193943)

No, it's fine then.

This revision was automatically updated to reflect the committed changes.