-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Enforce the compiler-builtins partitioning scheme #135395
base: master
Are you sure you want to change the base?
Conversation
…oboet Add #[inline] to copy_from_slice I'm doing cooked things to CGU partitioning for compiler-builtins (rust-lang#135395) and this was the lone symbol in my compiler-builtins rlib that wasn't an intrinsic. Adding `#[inline]` makes it go away. Perf report indicates a marginal but chaotic effect on compile time, marginal improvement in codegen. As expected.
7cf1a94
to
b371e7f
Compare
b371e7f
to
50dbf9c
Compare
r? bjorn3 |
This PR modifies cc @jieyouxu Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
// See https://github.com/rust-lang/rust/issues/73135 | ||
if tcx.is_compiler_builtins(rustc_span::def_id::LOCAL_CRATE) { | ||
return true; | ||
} |
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.
Is there a way to make inlining inside the crate more likely without causing MIR for all functions in compiler-builtins to get encoded in the crate metadata?
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.
I think what you're pointing out here is that these functions are not reachable as MIR, so we don't need to encode MIR for them. The problem as I see it is that our notion of reachable uses this worklist/visited algorithm that tracks items in a path-independent way:
rust/compiler/rustc_passes/src/reachable.rs
Lines 168 to 173 in 2ae9916
while let Some(search_item) = self.worklist.pop() { | |
if !scanned.insert(search_item) { | |
continue; | |
} | |
self.propagate_node(&self.tcx.hir_node_by_def_id(search_item), search_item); |
Also we already have an issue for the inverse inefficiency, emitting object code when we only need MIR: #119214
I put a hack in this place specifically because the compiler is designed around this function either true or false for whatever reason, past the first few checks. I'm not aware of anywhere else we could make a small localized change to get the behavior we want.
The only other place I could think of putting a hack is MonoItem::instantiation_mode
, but that doesn't work because then we get linker errors because instantiation mode needs to agree with exported_symbols
, and those disagree because because exported_symbols
is based on reachable_set
. I really think the inaccuracy of the reachable_set
analysis is the root problem here, and it's net better to implement this in a non-invasive way that will be fixed automatically if reachable_set
gets improved.
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.
Also, if I back up to my merge-base, x build library
, then ar x
the stage1-std libcompiler_builtins.rlib and run du -sch *
I get:
808K lib.rmeta
5.7M total
Then with my changes:
968K lib.rmeta
4.1M total
So even though it's not perfect, this PR is still a net win.
@bors r+ rollup=never |
compiler-builtins needs every intrinsic in its own CGU. Currently, the compiler-builtins crate puts every intrinsic in its own inline module then
library/Cargo.toml
uses a profile override so that when we build the sysroot, compiler-builtins is built with morecodegen-units
than we have intrinsics, and partitioning never merges two intrinsics together. This approach does not work with-Zbuild-std
because the profile override gets ignored. And it's kludgey anyway, our own standard library should not be fighting with our own compiler in an attempt to override its behavior. We should change the compiler's behavior to do the right thing in the first place.So that's what this PR does. There's some light refactoring of the CGU partitioning code, then in 3 places I've added a check for
is_compiler_builtins
:cross_crate_inlinable
; every function in compiler-builtins that is not#[no_mangle]
is made cross-crate-inlinable, which ensures we do not run into problems inlining helpers into intrinsics such as compiler-builtins: Int trait functions are not inlined on wasm #73135That should ensure that we have one object file per intrinsic, and if optimizations are enabled, there should be no extra extra CGUs full of helper functions (which is what currently happens in the precompiled standard library we distribute, my nightly libcompiler_builtins.rlib for x86_64-unknown-linux-gnu has 174 CGUs and with this PR we have 150).