Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CMake] Create generic runtimes targets as needed #76096

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

petrhosek
Copy link
Member

We currently create a generic runtime target for all subbuilds. For example if we're building libcxx for aarch64-linux-gnu and x86_64-linux-gnu, we would create the cxx target that would depend on cxx-aarch64-linux-gnu and cxx-x86_64-linux-gnu. The current implementation creates the generic runtimes targets ahead of time which introduces an issue where different subbuilds enable different runtimes. We should instead create the generic targets as needed.

We currently create a generic runtime target for all subbuilds. For
example if we're building libcxx for aarch64-linux-gnu and
x86_64-linux-gnu, we would create the cxx target that would depend on
cxx-aarch64-linux-gnu and cxx-x86_64-linux-gnu. The current
implementation creates the generic runtimes targets ahead of time which
introduces an issue where different subbuilds enable different runtimes.
We should instead create the generic targets as needed.
@petrhosek
Copy link
Member Author

I'm not sure if anyone uses these generic targets at all, it might be easier to just remove them altogether. I created #76097 which does that, let me know if you have a preference.

@smeenai
Copy link
Collaborator

smeenai commented Dec 20, 2023

I thought we had some complaints from Apple the last time the behavior of these generic targets changed? Maybe @ldionne would remember the details.

I'm in favor of keeping the targets, to minimize workflow changes and the potential for reverts. I'm okay with this change, but I'd also like to understand the motivation a bit better. We were already only adding dependencies to the generic targets if we were going to build them; is the change to avoid creating a generic target at all when it's not needed? That'd only happen when LLVM_ENABLE_RUNTIMES specifies a runtime but every single runtime target overrides that variable to remove the runtime, right?

@petrhosek
Copy link
Member Author

I encountered this when trying to enable libc for a single (baremetal) subbuild, and as such I don't want to put it in the LLVM_ENABLE_RUNTIMES global variable (that would result in the Darwin subbuild trying to build libc which is going to fail), but that means the generic libc target won't be created and the latter attempt at adding the dependency is going to fail here:

add_dependencies(${runtime_name} ${runtime_name}-${name})

More generally, I think that the global LLVM_ENABLE_RUNTIMES variable should only apply to the default subbuild:

function(runtime_default_target)

All other subbuilds should use RUNTIME_<target>_LLVM_ENABLE_RUNTIMES:
function(runtime_register_target name)

Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot about the opposite case, where you're only adding a runtime for a specific target. This LGTM then.

@ldionne
Copy link
Member

ldionne commented Dec 21, 2023

I don't remember that breakage, unfortunately. We shouldn't be using the bootstrapping build internally, so I don't think this should impact us, and I don't have any objection against this change.

@petrhosek petrhosek merged commit ed3e007 into llvm:main Jan 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants