Page MenuHomePhabricator

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

Authored by phosek on Thu, Oct 15, 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_BUILTINS_ONLY set.

Diff Detail

Unit TestsFailed

TimeTest
100 mslinux > Polly.ScopInfo/NonAffine::non-affine-loop-condition-dependent-access_3.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/NonAffine -polly-codegen-verify -basic-aa -polly-scops -polly-allow-nonaffine-branches -polly-allow-nonaffine-loops=false -analyze < /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/NonAffine/non-affine-loop-condition-dependent-access_3.ll | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/NonAffine/non-affine-loop-condition-dependent-access_3.ll --check-prefix=INNERMOST
230 mswindows > LLVM.tools/llvm-cov::warnings.h
Script: -- : 'RUN: at line 1'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\llvm-cov.exe show C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/prevent_false_instantiations.covmapping -instr-profile C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov | c:\ws\w32-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov\warnings.h -allow-empty -check-prefix=FAKE-FILE-STDOUT
470 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w32-1\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w32-1\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

phosek created this revision.Thu, Oct 15, 12:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptThu, Oct 15, 12:16 PM
Herald added subscribers: llvm-commits, Restricted Project, s.egerton and 3 others. · View Herald Transcript
phosek requested review of this revision.Thu, Oct 15, 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
49–55
206

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

213–214

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

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

We probably should retain the new cmake minimum

llvm/runtimes/CMakeLists.txt
415

We should probably disable the crt build here now.

528–529

Ditto the other comment

phosek updated this revision to Diff 298559.Fri, Oct 16, 12:22 AM
phosek marked 6 inline comments as done.
daltenty accepted this revision.Mon, Oct 19, 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
126

fyi, this needs a slight adjustments because D87120 has landed

This revision is now accepted and ready to land.Mon, Oct 19, 3:11 PM