[LLVM][runtimes] Prepopulate LLVM_BUILTIN_TARGETS with runtimes values
#95554
Conversation
|
Also, @petrhosek, what's the specific reason we need to build these in a separate CMake invocation? Doesn't the list of runtimes have an implicit order which we build them? I think we could add a dependency for the |
|
@jhuber6 the reason I initially implemented builtins and runtimes separately is because the runtime configuration completing successfully and accurately depends on the builtin libraries being available during configuration. We could probably make some educated guess hard-coded assumptions about what the toolchain supports, but you can quickly get off the rails with more exotic target support. |
I see, so it's related to the CMake configure step when it tries to identify features? I can see that being an issue for really specific checks where the compiler defaults to compiler-rt for its runtime instead of some GNU libs. Still unfortunate that it adds such a large overhead to build times. |
To make the point, who has GNU libs at all? I don't in 3 of the 4 configurations I run LLVM's build for. Yes it is unfortunate, but there are more build configurations than we can reasonably hard code predictions for. |
Alright, good to know the motivation behind it. In any case, this patch should fix it to where users will have the builtins built without needing to manually specify an extra variable. |
|
Ping, this is required for #95304. |
|
I think that @petrhosek and @llvm-beanz definitely should have a look over this - they have been working on the runtimes build, and would have a better idea of how they would like to see it evolve. |
|
I think this is probably fine. I think @petrhosek's team is probably the most impacted here, and it has been a few years since I was too involved in this area. From looking at the Fuchsia in-tree CMake cache scripts it looks like they explicitly set both builtin and runtime configuration values so this shouldn't break anything. @petrhosek any thoughts? |
| if("compiler-rt" IN_LIST LLVM_ENABLE_RUNTIMES) | ||
| list(APPEND builtin_targets "default") | ||
| endif() |
There was a problem hiding this comment.
I'm not sure that applying DRY here is worth it. I'd rather just duplicate the builtin_default_target invocation and remove the string handling for "default".
There was a problem hiding this comment.
We need the following case to work,
-DLLVM_ENABLE_RUNTIMES=compiler-rt \
-DRUNTIMES_nvptx64-nvidia-cuda_LLVM_ENABLE_RUNTIMES=compiler-rt \
-DRUNTIMES_amdgcn-amd-amdhsa_LLVM_ENABLE_RUNTIMES=compiler-rt \
-DLLVM_RUNTIME_TARGETS="default;amdgcn-amd-amdhsa;nvptx64-nvidia-cuda"
Honestly I think we should get rid of the distinction between the "default" target, but that's a much larger refactoring I think @petrhosek wanted to do.
There was a problem hiding this comment.
The "default" target is also important for how the compiler-rt parts of this are built on Darwin, which is a mess.
There was a problem hiding this comment.
I'm not sure that applying DRY here is worth it. I'd rather just duplicate the
builtin_default_targetinvocation and remove the string handling for "default".
I have a WIP change that removes the "default" altogether since that part is a relic, but it needs some more testing to make sure I don't break any of existing configurations.
|
ping |
llvm/runtimes/CMakeLists.txt
Outdated
| set(all_runtimes ${runtimes}) | ||
| foreach(name ${LLVM_RUNTIME_TARGETS}) | ||
| list(APPEND all_runtimes RUNTIMES_${name}_LLVM_ENABLE_RUNTIMES) | ||
| endforeach() | ||
| foreach(entry ${all_runtimes}) |
There was a problem hiding this comment.
This looks like an unrelated change that could land separately.
Summary: We create the builtins separately because these are required to set up before others are built. It's configured with a default value if not specified, but this doesn't respect the runtimes from other targets. This patch changes the behavior to prepopulate the builtins list with all the targets that have `compiler-rt` enabled if not overridden by the user.
…ues (llvm#95554) Summary: We create the builtins separately because these are required to set up before others are built. It's configured with a default value if not specified, but this doesn't respect the runtimes from other targets. This patch changes the behavior to prepopulate the builtins list with all the targets that have `compiler-rt` enabled if not overridden by the user.
Summary:
We create the builtins separately because these are required to set up
before others are built. It's configured with a default value if not
specified, but this doesn't respect the runtimes from other targets.
This patch changes the behavior to prepopulate the builtins list with
all the targets that have
compiler-rtenabled if not overridden by theuser.