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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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 |
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 |
compiler-rt/cmake/builtin-config-ix.cmake | ||
---|---|---|
207 | 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? |
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.
I also think separating this patch to following 3 parts makes applying this patch easy.
- moving AIX related definitions
- isolating crt-config-ix.cmake
- supporting bootstrap build of crtbegin/crtend and builtins
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!
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.
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.
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)
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...
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!
Thank you!
This patch looks like very clean now. And, it works lika a charm on VE. LGTM.
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!
ping. Is there anything I can help? I'd like to merge this patch as soon as possible. :-)
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,
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 ;-)
Thank you. I don't notice that this problem has been already fixed. Now compiling llvm for VE is much easier.
fyi, this needs a slight adjustments because D87120 has landed