-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Allow linking a prebuilt optimized compiler-rt builtins library #143689
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
base: master
Are you sure you want to change the base?
Allow linking a prebuilt optimized compiler-rt builtins library #143689
Conversation
This PR modifies If appropriate, please update This PR modifies If appropriate, please update
cc @tgross35 |
This comment has been minimized.
This comment has been minimized.
27fcb13
to
1ea1789
Compare
I don't really know much about the optimized compiler-rt builtins. Maybe @tgross35 might know more? |
Can you say more about how much time this saves? Historically it's been my impression locally that building is basically free, this flag is more about whether it's possible to build (e.g., needs to be off if you're ad-hoc cross compiling). Though maybe I'm remembering wrong? |
The goal is to enable optimized compiler-builtins in fedora packaging. There, rust is built using the distro provided llvm and resulting libraries. Some runtime features are absent if optimized compiler-builtins are not used. Notably, outlined aarch64 LSE atomic operations are not available without this support. |
Right, it's less about time and more about wanting to use our existing system LLVM packages. |
Ping @tgross35. The intent here is to enable optimized-builtins in Fedora using the system's compiler-rt.builtins library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to compiler-builtins lgtm with a possible rename of the var, assuming you have verified this works locally. Somebody more familiar with bootstrap (@Kobzol?) will need to review the changes there.
// NOTE: this interacts strangely with `llvm-has-rust-patches`. In that case, we enforce `submodules = false`, so this is a no-op. | ||
// But, the user could still decide to manually use an in-tree submodule. | ||
// | ||
// NOTE: if we're using system llvm, we'll end up building a version of `compiler-rt` that doesn't match the LLVM we're linking to. | ||
// That's probably ok? At least, the difference wasn't enforced before. There's a comment in | ||
// the compiler_builtins build script that makes me nervous, though: | ||
// https://github.com/rust-lang/compiler-builtins/blob/31ee4544dbe47903ce771270d6e3bea8654e9e50/build.rs#L575-L579 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: rewrap comments to 100 chars
// Optionally, link against a prebuilt compiler-rt library to supply | ||
// optimized intrinsics instead of compiling a subset of compiler-rt | ||
// from source. | ||
let link_against_prebuilt_rt = env::var_os("LLVM_BUILTIN_RT_LIB").is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe LLVM_COMPILER_RT_LIB
to match RUST_COMPILER_RT_ROOT
, and since this is probably linking the entire compiler-rt rather than just builtins. Also I believe the LLVM library is called "builtins" rather than "builtin"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of fedora, compiler-rt builtins is packaged as a distinct library. I plan on linking against it. Looking at build.rs
, on some targets (e.x openbsd), is only a broader compiler-rt library available to link against?
Extend the <target>.optimized-compiler-builtins bootstrap option to accept a path to a prebuilt compiler-rt builtins library, and update compiler-builtins to enable optimized builtins without building compiler-rt builtins.
1ea1789
to
8ce8e15
Compare
Extend the .optimized-compiler-builtins bootstrap option to accept a path to a prebuilt compiler-rt builtins library, and update compiler-builtins to enable optimized builtins without building compiler-rt builtins.