Repository navigation
Introduce OptimizationObjective and fix the entry shim's size attributes - #164784
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? mir-opt |
|
please pull the refactoring changes out into a separate commit. do we have anything in the perf test suite that actually uses z or s? I'd love to have some number we change in the perf test suite to track this over time |
|
Reminder, once the PR becomes ready for a review, use |
-Copt-level=s/z is most important for embedded applications where the total flash usage (combined size of all sections that will be mapped into memory, so no debuginfo, symbols, ... Generally only .text, .rodata and .data matter in the end) matters, not the size of rlibs or the size of the ELF files of the executable. |
Alright, I will split this PR into two. |
Understood; I will take a look at the perf test suite. Currently, I am conducting tests on my local machine using crates sourced from crates.io. |
I will conduct further testing on these sections. |
|
I don't think this is the right way to improve the size of executables complied with levels s or z. If LLVM is making the wrong inlining decisions we should have an explanation for why LLVM cannot adapt its heuristics to make the right decisions, before we start fighting with its inliner. What you are trying to do here could be a good idea, but I just want to point out that I have prototyped this change myself and discarded it because I concluded that the MIR inliner is likely to regress code size as often as it helps, because it isn't actually addressing the root cause of the problem inside LLVM. |
Hi. I want to separate two questions, because I think they get conflated. One is whether the inliner makes good decisions and whether it grows code. The other is whether it is right that the MIR inliner has never run at On the second: at On the first: I agree the current state is not good, and I am not claiming this change makes executables smaller. LLVM's inliner gives a bonus to a call that is the only remaining call to a local function, because inlining it is known to reduce size. The MIR inliner has no equivalent; it only rewards a callee whose own body contains a single call. The thresholds are not aligned either: at |
4710fdd to
706ca60
Compare
os/oz
This comment has been minimized.
This comment has been minimized.
706ca60 to
81c0b8e
Compare
|
@rustbot ready |
…uwer Rollup of 19 pull requests Successful merges: - #164784 (Introduce OptimizationObjective and fix the entry shim's size attributes) - #165037 (Drop array elements in order after a partial move) - #165078 (Use the `2.38` binutils version in CI) - #159374 (Lower #[no_mangle] to #[symbol_name]/#[link_name]) - #163284 (feat(Reborrow): Allow multiple lifetimes) - #164775 (Tweak output on misused `const` parameter to provide more context and more accurate suggestion) - #165194 (name elided lifetimes in `use<...>` suggestion when `impl Trait` params are present) - #165222 (Anonymize inner binders when checking refining) - #165335 (Fallible type ops) - #165388 (Accept variadic SysV64, Win64, and Aapcs FnAbis in rustc_public) - #165448 (Update `Vec::retain_unstable` docs to say that this method is generally faster than `Vec::retain_mut`) - #165494 (fix Miri ICE on exit with () return type) - #165503 (update Enzyme submodule) - #165505 (DFV: Reject `#[non_exhaustive]` on enum variants with default field values) - #165512 (Fix warnings for the L4Re targets) - #165514 (Remove unused `WhereEqPredicate` and `ScopeId` types) - #165518 (style-guide: Remove ABI explicitness prescription for extern items) - #165519 (Update books) - #165534 (rename MIR_borrow_checking timing scope to less confusing name)
This comment has been minimized.
This comment has been minimized.
81c0b8e to
4efd167
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
This PR was contained in a rollup (#165548), which was closed. |
|
@rustbot ready |
|
@bors r+ |
…i-obk Introduce OptimizationObjective and fix the entry shim's size attributes ~~The MIR inliner's 2 => arm allow-lists `More | Aggressive` by hand. `s/z` also derive MIR level 2, so the pass never runs there. I could not find why in the source or the history — the gate predates the tiered match and read `mir_opt_level() >= 3`, which `s/z` never satisfied either, and `Size/SizeMin` simply were not among the entries considered when the match was written. There is no revert or regression behind it. Dumping every MIR pass at all four levels, the pass sets at `2/3` and at `s/z` differ by exactly one name: Inline. So I enabled it through `PassPolicy::size_only()` and measured.~~ ~~The general situation is as follows:~~ ~~For the `.text` section in `rlib` files, I tested 24 crates; the results were −3.59% (17/24) for `os` and −5.78% (16/24) for `oz`. Additional test data includes:~~ ~~76-crate sample, Σ stripped: −0.08% (`os`), −0.13% (`oz`)~~ ~~22 apps, Σ stripped: −0.09% (`os`), −0.07% (`oz`)~~ ~~22 apps, on-disk bytes: −0.40% (7/22) (`os`), −0.74% (11/22) for (`oz`)~~ ~~22 apps, Σ .text: +0.18% (`os`), +0.84% (`oz`)~~ ~~Is enabling MIR inlining at `s/z` the right direction, given that it clearly shrinks libraries and application binaries on disk while growing application `.text`?~~ *Update*: as @oli-obk asked, the refactoring is now its own commit. The MIR inliner change will move out to another PR. The measurements above are why I still think it is worth doing, but it does not belong in this PR. When I Split it out I found that LLVM 23 no longer has `Os/Oz` as pipeline levels, they both build the `O2` pipeline now, and what marks a function as size-optimized is the `optsize/minsize` attribute on it. So at `-Copt-level=s/z` there is no longer any setting rustc hands to LLVM that says "this is a size build", it is entirely per-function. It exposed a gap that the C entry shim and `__rust_try` have no item behind them and never go through the per-item attribute path, so they were the only definitions in a size build that LLVM optimized for speed. They now get the same attributes as everything else. The refactoring itself gives size-versus-speed a name and moves the sites that are really asking "what is this build optimizing for" onto it. The ones that are asking "how hard" — the linker and pipeline level tables, where `s` and `z` map to different strings, still match on `OptLevel`. `CodeGenOptSize` was already being discarded at its only call site, so it is gone. There are codegen tests for each knob that now reads the objective, at `2`, `s` and `z`.
Rollup of 5 pull requests Successful merges: - #164784 (Introduce OptimizationObjective and fix the entry shim's size attributes) - #147696 (Implement `bit` and `set_bit` for integral types.) - #163202 (nuttx: use updated pollfd definition) - #163571 ([rustdoc] Render `Deref` methods on aliased items) - #165482 (apply `-Zcrate-attr` attributes before the crate attributes)
…i-obk Introduce OptimizationObjective and fix the entry shim's size attributes ~~The MIR inliner's 2 => arm allow-lists `More | Aggressive` by hand. `s/z` also derive MIR level 2, so the pass never runs there. I could not find why in the source or the history — the gate predates the tiered match and read `mir_opt_level() >= 3`, which `s/z` never satisfied either, and `Size/SizeMin` simply were not among the entries considered when the match was written. There is no revert or regression behind it. Dumping every MIR pass at all four levels, the pass sets at `2/3` and at `s/z` differ by exactly one name: Inline. So I enabled it through `PassPolicy::size_only()` and measured.~~ ~~The general situation is as follows:~~ ~~For the `.text` section in `rlib` files, I tested 24 crates; the results were −3.59% (17/24) for `os` and −5.78% (16/24) for `oz`. Additional test data includes:~~ ~~76-crate sample, Σ stripped: −0.08% (`os`), −0.13% (`oz`)~~ ~~22 apps, Σ stripped: −0.09% (`os`), −0.07% (`oz`)~~ ~~22 apps, on-disk bytes: −0.40% (7/22) (`os`), −0.74% (11/22) for (`oz`)~~ ~~22 apps, Σ .text: +0.18% (`os`), +0.84% (`oz`)~~ ~~Is enabling MIR inlining at `s/z` the right direction, given that it clearly shrinks libraries and application binaries on disk while growing application `.text`?~~ *Update*: as @oli-obk asked, the refactoring is now its own commit. The MIR inliner change will move out to another PR. The measurements above are why I still think it is worth doing, but it does not belong in this PR. When I Split it out I found that LLVM 23 no longer has `Os/Oz` as pipeline levels, they both build the `O2` pipeline now, and what marks a function as size-optimized is the `optsize/minsize` attribute on it. So at `-Copt-level=s/z` there is no longer any setting rustc hands to LLVM that says "this is a size build", it is entirely per-function. It exposed a gap that the C entry shim and `__rust_try` have no item behind them and never go through the per-item attribute path, so they were the only definitions in a size build that LLVM optimized for speed. They now get the same attributes as everything else. The refactoring itself gives size-versus-speed a name and moves the sites that are really asking "what is this build optimizing for" onto it. The ones that are asking "how hard" — the linker and pipeline level tables, where `s` and `z` map to different strings, still match on `OptLevel`. `CodeGenOptSize` was already being discarded at its only call site, so it is gone. There are codegen tests for each knob that now reads the objective, at `2`, `s` and `z`.
…uwer Rollup of 8 pull requests Successful merges: - #164784 (Introduce OptimizationObjective and fix the entry shim's size attributes) - #165469 (Make notable trait `i` badges `<span>` instead of `<a>`) - #165576 (compiletest: Reject whitespace between `//` & `@` and reject leading revision conditions like (e.g., `//[rev]@ dir`)) - #147696 (Implement `bit` and `set_bit` for integral types.) - #163202 (nuttx: use updated pollfd definition) - #165482 (apply `-Zcrate-attr` attributes before the crate attributes) - #165517 (Migrate basic block ctors to require Terminator where possible) - #165552 (use lang item instead of name when assembling builtin `Clone::clone` instances)
|
⌛ Testing commit 4efd167 with merge f1865dc... Workflow: https://github.com/rust-lang/rust/actions/runs/42393727046 |
Introduce OptimizationObjective and fix the entry shim's size attributes ~~The MIR inliner's 2 => arm allow-lists `More | Aggressive` by hand. `s/z` also derive MIR level 2, so the pass never runs there. I could not find why in the source or the history — the gate predates the tiered match and read `mir_opt_level() >= 3`, which `s/z` never satisfied either, and `Size/SizeMin` simply were not among the entries considered when the match was written. There is no revert or regression behind it. Dumping every MIR pass at all four levels, the pass sets at `2/3` and at `s/z` differ by exactly one name: Inline. So I enabled it through `PassPolicy::size_only()` and measured.~~ ~~The general situation is as follows:~~ ~~For the `.text` section in `rlib` files, I tested 24 crates; the results were −3.59% (17/24) for `os` and −5.78% (16/24) for `oz`. Additional test data includes:~~ ~~76-crate sample, Σ stripped: −0.08% (`os`), −0.13% (`oz`)~~ ~~22 apps, Σ stripped: −0.09% (`os`), −0.07% (`oz`)~~ ~~22 apps, on-disk bytes: −0.40% (7/22) (`os`), −0.74% (11/22) for (`oz`)~~ ~~22 apps, Σ .text: +0.18% (`os`), +0.84% (`oz`)~~ ~~Is enabling MIR inlining at `s/z` the right direction, given that it clearly shrinks libraries and application binaries on disk while growing application `.text`?~~ *Update*: as @oli-obk asked, the refactoring is now its own commit. The MIR inliner change will move out to another PR. The measurements above are why I still think it is worth doing, but it does not belong in this PR. When I Split it out I found that LLVM 23 no longer has `Os/Oz` as pipeline levels, they both build the `O2` pipeline now, and what marks a function as size-optimized is the `optsize/minsize` attribute on it. So at `-Copt-level=s/z` there is no longer any setting rustc hands to LLVM that says "this is a size build", it is entirely per-function. It exposed a gap that the C entry shim and `__rust_try` have no item behind them and never go through the per-item attribute path, so they were the only definitions in a size build that LLVM optimized for speed. They now get the same attributes as everything else. The refactoring itself gives size-versus-speed a name and moves the sites that are really asking "what is this build optimizing for" onto it. The ones that are asking "how hard" — the linker and pipeline level tables, where `s` and `z` map to different strings, still match on `OptLevel`. `CodeGenOptSize` was already being discarded at its only call site, so it is gone. There are codegen tests for each knob that now reads the objective, at `2`, `s` and `z`.
|
@bors yield |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #165597. |
…uwer Rollup of 8 pull requests Successful merges: - #164784 (Introduce OptimizationObjective and fix the entry shim's size attributes) - #165469 (Make notable trait `i` badges `<span>` instead of `<a>`) - #165576 (compiletest: Reject whitespace between `//` & `@` and reject leading revision conditions like (e.g., `//[rev]@ dir`)) - #147696 (Implement `bit` and `set_bit` for integral types.) - #163202 (nuttx: use updated pollfd definition) - #165482 (apply `-Zcrate-attr` attributes before the crate attributes) - #165517 (Migrate basic block ctors to require Terminator where possible) - #165552 (use lang item instead of name when assembling builtin `Clone::clone` instances)
Rollup merge of #164784 - cezarbbb:overhaul-size-flags, r=oli-obk Introduce OptimizationObjective and fix the entry shim's size attributes ~~The MIR inliner's 2 => arm allow-lists `More | Aggressive` by hand. `s/z` also derive MIR level 2, so the pass never runs there. I could not find why in the source or the history — the gate predates the tiered match and read `mir_opt_level() >= 3`, which `s/z` never satisfied either, and `Size/SizeMin` simply were not among the entries considered when the match was written. There is no revert or regression behind it. Dumping every MIR pass at all four levels, the pass sets at `2/3` and at `s/z` differ by exactly one name: Inline. So I enabled it through `PassPolicy::size_only()` and measured.~~ ~~The general situation is as follows:~~ ~~For the `.text` section in `rlib` files, I tested 24 crates; the results were −3.59% (17/24) for `os` and −5.78% (16/24) for `oz`. Additional test data includes:~~ ~~76-crate sample, Σ stripped: −0.08% (`os`), −0.13% (`oz`)~~ ~~22 apps, Σ stripped: −0.09% (`os`), −0.07% (`oz`)~~ ~~22 apps, on-disk bytes: −0.40% (7/22) (`os`), −0.74% (11/22) for (`oz`)~~ ~~22 apps, Σ .text: +0.18% (`os`), +0.84% (`oz`)~~ ~~Is enabling MIR inlining at `s/z` the right direction, given that it clearly shrinks libraries and application binaries on disk while growing application `.text`?~~ *Update*: as @oli-obk asked, the refactoring is now its own commit. The MIR inliner change will move out to another PR. The measurements above are why I still think it is worth doing, but it does not belong in this PR. When I Split it out I found that LLVM 23 no longer has `Os/Oz` as pipeline levels, they both build the `O2` pipeline now, and what marks a function as size-optimized is the `optsize/minsize` attribute on it. So at `-Copt-level=s/z` there is no longer any setting rustc hands to LLVM that says "this is a size build", it is entirely per-function. It exposed a gap that the C entry shim and `__rust_try` have no item behind them and never go through the per-item attribute path, so they were the only definitions in a size build that LLVM optimized for speed. They now get the same attributes as everything else. The refactoring itself gives size-versus-speed a name and moves the sites that are really asking "what is this build optimizing for" onto it. The ones that are asking "how hard" — the linker and pipeline level tables, where `s` and `z` map to different strings, still match on `OptLevel`. `CodeGenOptSize` was already being discarded at its only call site, so it is gone. There are codegen tests for each knob that now reads the objective, at `2`, `s` and `z`.
View all comments
The MIR inliner's 2 => arm allow-listsMore | Aggressiveby hand.s/zalso derive MIR level 2, so the pass never runs there. I could not find why in the source or the history — the gate predates the tiered match and readmir_opt_level() >= 3, whichs/znever satisfied either, andSize/SizeMinsimply were not among the entries considered when the match was written. There is no revert or regression behind it. Dumping every MIR pass at all four levels, the pass sets at2/3and ats/zdiffer by exactly one name: Inline. So I enabled it throughPassPolicy::size_only()and measured.The general situation is as follows:For the.textsection inrlibfiles, I tested 24 crates; the results were −3.59% (17/24) forosand −5.78% (16/24) foroz.Additional test data includes:
76-crate sample, Σ stripped: −0.08% (os), −0.13% (oz)22 apps, Σ stripped: −0.09% (os), −0.07% (oz)22 apps, on-disk bytes: −0.40% (7/22) (os), −0.74% (11/22) for (oz)22 apps, Σ .text: +0.18% (os), +0.84% (oz)Is enabling MIR inlining ats/zthe right direction, given that it clearly shrinks libraries and application binaries on disk while growing application.text?Update: as @oli-obk asked, the refactoring is now its own commit. The MIR inliner change will move out to another PR. The measurements above are why I still think it is worth doing, but it does not belong in this PR.
When I Split it out I found that LLVM 23 no longer has
Os/Ozas pipeline levels, they both build theO2pipeline now, and what marks a function as size-optimized is theoptsize/minsizeattribute on it. So at-Copt-level=s/zthere is no longer any setting rustc hands to LLVM that says "this is a size build", it is entirely per-function.It exposed a gap that the C entry shim and
__rust_tryhave no item behind them and never go through the per-item attribute path, so they were the only definitions in a size build that LLVM optimized for speed. They now get the same attributes as everything else.The refactoring itself gives size-versus-speed a name and moves the sites that are really asking "what is this build optimizing for" onto it. The ones that are asking "how hard" — the linker and pipeline level tables, where
sandzmap to different strings, still match onOptLevel.CodeGenOptSizewas already being discarded at its only call site, so it is gone.There are codegen tests for each knob that now reads the objective, at
2,sandz.