This is an archive of the discontinued LLVM Phabricator instance.

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

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
kaz7 added a comment.Dec 3 2021, 7:35 PM

Hi. Thank you for trying to achieve pre-compiling of crtbegin/crtend and builtins. Our architecture, SX-Aurora VE, doesn't have freely distributed crtbegin/crtend, so a mechanism like this is really important as I mentioned in D115038.

I take a look. I think the way to isolate each modules are good way to do. So, basically, it's fine to land in my opinion. However, we have been updating runtimes tramendously. So, it's better to rebase to recent main. We already have compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake. Scope is changed like @daltenty suggested.

After that, we can review it and run it. Thank you for your efforts.

kaz7 added a comment.Dec 3 2021, 8:00 PM

I also think separating this patch to following 3 parts makes applying this patch easy.

  1. moving AIX related definitions
  2. isolating crt-config-ix.cmake
  3. supporting bootstrap build of crtbegin/crtend and builtins
kaz7 added a comment.Dec 7 2021, 4:51 AM

I've tested this patch after rebasing it by hand. It works great. So, I really want to contribute whatever I can. @phosek , please let me know what ever I can help. Thanks!

simoll added a subscriber: simoll.Dec 9 2021, 4:45 AM
kaz7 added a comment.Dec 17 2021, 5:08 AM

Hi, is there anything I can help to merge this patch? We need this modification to compile crtbegin/crtend before runtime libraries in order to compile LLVM for VE using bootstrapping build, https://libcxx.llvm.org/BuildingLibcxx.html#bootstrapping-build.

kaz7 added a comment.Feb 11 2022, 3:28 PM

Hi @phosek

I understand you are very busy. But, is there any way to push forward this patch since the ability of runtime standalone build has been removed at D119255. We, VE, badly need this feature, compiling crtbegin.o and crtend.o before runtimes. I previously posted similar patch, D115038, but it is recommended to remove since you already post this patch. But, this patch is not merged yet. Now, this patch is blocking VE runtime build from my point of view. Is it OK to make a similar patch if you don't have time to work on this? Please let me know what you think. Thanks.

q66 added a subscriber: q66.Apr 11 2022, 7:35 PM

are there any plans to get this or an equivalent in? currently in my distribution i have to ship a version of this patch rebased to llvm 14 (https://github.com/chimera-linux/cports/blob/master/main/llvm/patches/999-build-runtimes-crt.patch) because if i don't, i can't build llvm at all (my system is fully llvm-based, therefore does not come with a gcc crt; an llvm crt is needed in order to build the rest of the runtimes, as otherwise invoking the in-tree clang used to build compiler-rt will try to find crtbeginS.o etc. which do not exist in my system)

Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 7:35 PM
q66 added a comment.Apr 11 2022, 7:37 PM

considering the old way of doing things is deprecated and as far as i can tell scheduled for removal in llvm 15, not having something equivalent to this means llvm will no longer build out of box in gcc-less linux systems...

smeenai resigned from this revision.Apr 28 2022, 2:05 PM
kaz7 added a comment.Aug 10 2022, 8:05 PM

Hi, is it possible to rebase this patch to the latest main branch? I think this patch is almost done since many pieces of this patch is already merged. I appreciate if you have time to rebase it and merge this patch to main. I, llvm for VE, definitely need this feature to compile llvm at once. Thanks!

Hi, is it possible to rebase this patch to the latest main branch? I think this patch is almost done since many pieces of this patch is already merged. I appreciate if you have time to rebase it and merge this patch to main. I, llvm for VE, definitely need this feature to compile llvm at once. Thanks!

I'll try to upload an updated patch this week.

phosek updated this revision to Diff 453217.Aug 17 2022, 12:56 AM
kaz7 accepted this revision.Aug 22 2022, 2:30 AM

Thank you!

This patch looks like very clean now. And, it works lika a charm on VE. LGTM.

kaz7 added a comment.Aug 28 2022, 12:48 AM

Hi @daltenty, is it possible to re-review this patch which you requested changes previously? This patch is holded because of your request, I think. Thank you!

kaz7 added a comment.Jan 12 2023, 2:09 PM

ping. Is there anything I can help? I'd like to merge this patch as soon as possible. :-)

kaz7 added a comment.May 20 2023, 8:41 AM

ping. Do I need to create a new patch copying this to merge this patch?

ping. Do I need to create a new patch copying this to merge this patch?

You've got an approval, just push the commit (assuming you have commit access). I guess you need to rebase it to resolve conflicts first.

ping. Do I need to create a new patch copying this to merge this patch?

You've got an approval, just push the commit (assuming you have commit access). I guess you need to rebase it to resolve conflicts first.

Ah, never mind. I thought you are the author of the patch.
I'm also interested in this change.

I apologize about the delay, this change still has some issues that I haven't had yet time to resolve, I plan to land D136664 in the meantime,

efocht added a subscriber: efocht.Aug 14 2023, 4:26 AM

As far as I can see https://reviews.llvm.org/D136664 was accepted on Nov 4, 2022 and can land.
Which would unblock this one (https://reviews.llvm.org/D89492). After almost 3 years we could actually send it to the Kindergarden ;-)

phosek abandoned this revision.Aug 14 2023, 11:46 PM

Superseded by D153989.

kaz7 added a comment.Aug 16 2023, 10:36 AM

Thank you. I don't notice that this problem has been already fixed. Now compiling llvm for VE is much easier.