Avoid duplicate `large_assignments` lints
By checking for overlapping spans.
This PR does the "reduce noisiness" task in #83518.
r? `@oli-obk` who added E-mentor and E-help-wanted and wrote the initial code.
(The fix itself is in dc82736677. The two commits before that are just small refactorings.)
Tweak CGU sorting in a couple of places.
In `base.rs`, tweak how the CGU size interleaving works. Since #113777, it's much more common to have multiple CGUs with identical sizes. With the existing code these same-sized items ended up in the opposite-to-desired order due to the stable sorting. The code now starts with a reverse sort (like is done in `partitioning.rs`) which gives the behaviour we want. This doesn't matter much for perf, but makes profiles in `samply` look more like what we expect.
In `partitioning.rs`, we can use `sort_by_key` instead of `sort_by_cached_key` because `CGU::size_estimate()` is cheap. (There is an identical CGU sort earlier in that function that already uses `sort_by_key`.)
r? `@pnkfelix`
In `base.rs`, tweak how the CGU size interleaving works. Since #113777,
it's much more common to have multiple CGUs with identical sizes. With
the existing code these same-sized items ended up in the
opposite-to-desired order due to the stable sorting. The code now starts
with a reverse sort (like is done in `partitioning.rs`) which gives the
behaviour we want. This doesn't matter much for perf, but makes profiles
in `samply` look more like what we expect.
In `partitioning.rs`, we can use `sort_by_key` instead of
`sort_by_cached_key` because `CGU::size_estimate()` is cheap. (There is
an identical CGU sort earlier in that function that already uses
`sort_by_key`.)
Instead of repeatedly merging the two smallest CGUs, we now use a
merging algorithm that aims to minimize the duplication of inlined
functions.
`exa-0.10.1` was one benchmark that saw particularly good results. The
old CGU stats:
```
INTERNALIZE
- unique items: 2774 (1216 root + 1558 inlined), unique size: 122065 (77219 root + 44846 inlined)
- placed items: 3834 (1216 root + 2618 inlined), placed size: 154552 (77219 root + 77333 inlined)
- placed/unique items ratio: 1.38, placed/unique size ratio: 1.27
- CGUs: 16, mean size: 9659.5, sizes: [11791, 11634, 11173, 10987, 10939, 10507, 9992, 9813, 9593, 9580, 9030, 8447, 7975, 7961, 7876, 7254]
```
The new CGU stats:
```
INTERNALIZE
- unique items: 2774 (1216 root + 1558 inlined), unique size: 122065 (77219 root + 44846 inlined)
- placed items: 3626 (1216 root + 2410 inlined), placed size: 147201 (77219 root + 69982 inlined)
- placed/unique items ratio: 1.31, placed/unique size ratio: 1.21
- CGUs: 16, mean size: 9200.1, sizes: [11634, 10939, 10227, 9555, 9178, 9167, 8879, 8804, 8604, 8603 (x3), 8602 (x2), 8601, 8600]
```
The difference is in the number of inlined items. There are 1558 unique
inlined items. With the old algorithm these were placed 2618 times,
resulting in 1060 duplicates. With the new algorithm these were placed
2410 times, resulting in 852 duplicates. Also, the mean CGU size dropped
from 9659.5 to 9200.1, and the CGU size distribution tightened, with the
biggest one a little smaller and the smallest ones a little bigger.
They're quite rare, and ignoring them simplifies things quite a bit, and
further reduces the number of calls to `MonoItem::size_estimate` to the
number of placed items (one per root item, and one or more per reachable
inlined item).
This means we call `MonoItem::size_estimate` (which involves a query)
less often: just once per mono item, and then once more per inline item
placement. After that we can reuse the stored value as necessary. This
means `CodegenUnit::compute_size_estimate` is cheaper.
Rollup of 7 pull requests
Successful merges:
- #112931 (Enable zlib in LLVM on aarch64-apple-darwin)
- #113158 (tests: unset `RUSTC_LOG_COLOR` in a test)
- #113173 (CI: include workflow name in concurrency group)
- #113335 (Reveal opaques in new solver)
- #113390 (CGU formation tweaks)
- #113399 (Structurally normalize again for byte string lit pat checking)
- #113412 (Add basic types to SMIR)
r? `@ghost`
`@rustbot` modify labels: rollup
It makes it sound like the `ExprKind` and `Rvalue` are supposed to represent all pointer related
casts, when in reality their just used to share a some enum variants. Make it clear there these
are only coercion to make it clear why only some pointer related "casts" are in the enum.
For non-incremental builds on Unix, currently all the thread names look
like `opt regex.f10ba03eb5ec7975-cgu.0`. But they are truncated by
`pthread_setname` to `opt regex.f10ba`, hiding the numeric suffix that
distinguishes them. This is really annoying when using a profiler like
Samply.
This commit changes these thread names to a form like `opt cgu.0`, which
is much better.
Currently there are two problems.
First, the CGUS don't end up in size order. The merging loop does sort
by size on each iteration, but we don't sort after the final merge, so
typically there is one CGU out of place. (And sometimes we don't enter
the merging loop at all, in which case they end up in random order.)
Second, we then assign names that differ only by a numeric suffix, and
then we sort them lexicographically by name, giving us an order like
this:
regex.f10ba03eb5ec7975-cgu.1
regex.f10ba03eb5ec7975-cgu.10
regex.f10ba03eb5ec7975-cgu.11
regex.f10ba03eb5ec7975-cgu.12
regex.f10ba03eb5ec7975-cgu.13
regex.f10ba03eb5ec7975-cgu.14
regex.f10ba03eb5ec7975-cgu.15
regex.f10ba03eb5ec7975-cgu.2
regex.f10ba03eb5ec7975-cgu.3
regex.f10ba03eb5ec7975-cgu.4
regex.f10ba03eb5ec7975-cgu.5
regex.f10ba03eb5ec7975-cgu.6
regex.f10ba03eb5ec7975-cgu.7
regex.f10ba03eb5ec7975-cgu.8
regex.f10ba03eb5ec7975-cgu.9
These two problems are really annoying when debugging and profiling the
CGUs.
This commit ensures CGUs are sorted by name *and* reverse sorted by
size. This involves (a) one extra sort by size operation, and (b)
padding the numeric indices with zeroes, e.g.
`regex.f10ba03eb5ec7975-cgu.01`.
(Note that none of this applies for incremental builds, where a
different hash-based CGU naming scheme is used.)
- Rename `create_size_estimate` as `compute_size_estimate`, because that
makes more sense for the second and subsequent calls for each CGU.
- Change `CodegenUnit::size_estimate` from `Option<usize>` to `usize`.
We can still assert that `compute_size_estimate` is called first.
- Move the size estimation for `place_mono_items` inside the function,
for consistency with `merge_codegen_units`.
Because CGU merging relies on CGU sizes, but the CGU sizes before
inlining aren't accurate.
This requires tweaking how the sizes are updated during merging: if CGU
A and B both have an inlined function F, then `size(A + B)` will be a
little less than `size(A) + size(B)`, because `A + B` will only have one
copy of F. Also, the minimum CGU size is increased because it now has to
account for inlined functions.
This change doesn't have much effect on compile perf, but it makes
follow-on changes that involve more sophisticated reasoning about CGU
sizes much easier.
Remove `box_free` lang item
This PR removes the `box_free` lang item, replacing it with `Box`'s `Drop` impl. Box dropping is still slightly magic because the contained value is still dropped by the compiler.
Always put the `create_size_estimate` calls and `debug_dump` calls
within a timed scopes. This makes the four main steps look more similar
to each other.
The comment says "Find the smallest CGU that has exported symbols and
put the dead function stubs in that CGU". But the code sorts the CGUs by
size (smallest first) and then searches them in reverse order, which
means it will find the *largest* CGU that has exported symbols.
The erroneous code was introduced in #92142.
This commit changes it to use a simpler search, avoiding the sort, and
fixes the bug in the process.
Because tiny CGUs make compilation less efficient *and* result in worse
generated code.
We don't do this when the number of CGUs is explicitly given, because
there are times when the requested number is very important, as
described in some comments within the commit. So the commit also
introduces a `CodegenUnits` type that distinguishes between default
values and user-specified values.
This change has a roughly neutral effect on walltimes across the
rustc-perf benchmarks; there are some speedups and some slowdowns. But
it has significant wins for most other metrics on numerous benchmarks,
including instruction counts, cycles, binary size, and max-rss. It also
reduces parallelism, which is good for reducing jobserver competition
when multiple rustc processes are running at the same time. It's smaller
benchmarks that benefit the most; larger benchmarks already have CGUs
that are all larger than the minimum size.
Here are some example before/after CGU sizes for opt builds.
- html5ever
- CGUs: 16, mean size: 1196.1, sizes: [3908, 2992, 1706, 1652, 1572,
1136, 1045, 948, 946, 938, 579, 471, 443, 327, 286, 189]
- CGUs: 4, mean size: 4396.0, sizes: [6706, 3908, 3490, 3480]
- libc
- CGUs: 12, mean size: 35.3, sizes: [163, 93, 58, 53, 37, 8, 2 (x6)]
- CGUs: 1, mean size: 424.0, sizes: [424]
- tt-muncher
- CGUs: 5, mean size: 1819.4, sizes: [8508, 350, 198, 34, 7]
- CGUs: 1, mean size: 9075.0, sizes: [9075]
Note that CGUs of size 100,000+ aren't unusual in larger programs.
This loop is doing two different things. For inlined items, it's adding
them to the CGU. For all items, it's recording them in
`mono_item_placements`.
This commit splits it into two separate loops. This avoids putting root
mono items into `reachable`, and removes the low-value check that
`roots` doesn't contain inlined mono items.
Currently it sorts by symbol name, which is a mangled name like
`_ZN1a4main17hb29587cdb6db5f42E`, which leads to non-obvious orderings.
This commit changes it to use the existing
`items_in_deterministic_order`, which iterates in source code order.
I found this confusing because it includes the root item, plus the
inlined items reachable from the root item. The new formulation
separates the two parts more clearly.
Currently it overwrites all the CGUs with new CGUs. But those new CGUs
are just copies of the old CGUs, possibly with some things added. This
commit changes things so that each CGU just gets added to in place,
which makes things simpler and clearer.
It currently uses ranges, which index into `UsageMap::used_items`. This
commit changes it to just use `Vec`, which is much simpler to construct
and use. This change does result in more allocations, but it is few
enough that the perf impact is negligible.
`UsageMap` contains `used_map`, which maps from an item to the item it
uses. This commit add `user_map`, which is the inverse.
We already compute this inverse, but later on, and it is only held as a
local variable. Its simpler and nicer to put it next to `used_map`.
Currently, the code uses multiple words to describe when a mono item `f`
uses a mono item `g`, all of which have problems.
- `f` references `g`: confusing because there are multiple kinds of use,
e.g. "`f` calls `g`" is one, but "`f` takes a (`&T`-style) reference
of `g`" is another, and that's two subtly different meanings of
"reference" in play.
- `f` accesses `g`: meh, "accesses" makes me think of data, and this is
code.
- `g` is a neighbor (or neighbour) of `f`: is verbose, and doesn't
capture the directionality.
This commit changes the code to use "`f` uses `g`" everywhere. I think
it's better than the current terminology, and the consistency is
important.
Also, `InliningMap` is renamed `UsageMap` because (a) it was always
mostly about usage, and (b) the inlining information it did record was
removed in a recent commit.
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
Remove `-Zcgu-partitioning-strategy`.
This option was introduced three years ago, but it's never been meaningfully used, and `default` is the only acceptable value.
Also, I think the `Partition` trait presents an interface that is too closely tied to the existing strategy and would probably be wrong for other strategies. (My rule of thumb is to not make something generic until there are at least two instances of it, to avoid this kind of problem.)
Also, I don't think providing multiple partitioning strategies to the user is a good idea, because the compiler already has enough obscure knobs.
This commit removes the option, along with the `Partition` trait, and the `Partitioner` and `DefaultPartitioning` types. I left the existing code in `compiler/rustc_monomorphize/src/partitioning/default.rs`, though I could be persuaded that moving it into
`compiler/rustc_monomorphize/src/partitioning/mod.rs` is better.
r? ``@wesleywiser``
This option was introduced three years ago, but it's never been
meaningfully used, and `default` is the only acceptable value.
Also, I think the `Partition` trait presents an interface that is too
closely tied to the existing strategy and would probably be wrong for
other strategies. (My rule of thumb is to not make something generic
until there are at least two instances of it, to avoid this kind of
problem.)
Also, I don't think providing multiple partitioning strategies to the
user is a good idea, because the compiler already has enough obscure
knobs.
This commit removes the option, along with the `Partition` trait, and
the `Partitioner` and `DefaultPartitioning` types. I left the existing
code in `compiler/rustc_monomorphize/src/partitioning/default.rs`,
though I could be persuaded that moving it into
`compiler/rustc_monomorphize/src/partitioning/mod.rs` is better.
I find that these structs obfuscate the code. Removing them and just
passing the individual fields around makes the `Partition` method
signatures a little longer, but makes the data flow much clearer. E.g.
- `codegen_units` is mutable all the way through.
- `codegen_units`'s length is changed by `merge_codegen_units`, but only
the individual elements are changed by `place_inlined_mono_items` and
`internalize_symbols`.
- `roots`, `internalization_candidates`, and `mono_item_placements` are
all immutable after creation, and all used by just one of the four
methods.
Three of the four methods in `DefaultPartitioning` are defined in
`default.rs`. But `merge_codegen_units` is defined in a separate module,
`merging`, even though it's less than 100 lines of code and roughly the
same size as the other three methods. (Also, the `merging` module
currently sits alongside `default`, when it should be a submodule of
`default`, adding to the confusion.)
In #74275 this explanation was given:
> I pulled this out into a separate module since it seemed like we might
> want a few different merge algorithms to choose from.
But in the three years since there have been no additional merging
algorithms, and there is no mechanism for choosing between different
merging algorithms. (There is a mechanism,
`-Zcgu-partitioning-strategy`, for choosing between different
partitioning strategies, but the merging algorithm is just one piece of
a partitioning strategy.)
This commit merges `merging` into `default`, making the code easier to
navigate and read.
- Pass a slice instead of an iterator to `debug_dump`.
- For each CGU set, print: the number of CGUs, the max and min size, and
the ratio of the max and min size (which indicates how evenly sized
they are).
- Print a `FINAL` entry, showing the absolute final results.
Run various queries from other queries instead of explicitly in phases
These are just legacy leftovers from when rustc didn't have a query system. While there are more cleanups of this sort that can be done here, I want to land them in smaller steps.
This phased order of query invocations was already a lie, as any query that looks at types (e.g. the wf checks run before) can invoke e.g. const eval which invokes borrowck, which invokes typeck, ...
Remove some uses of dynamic dispatch during monomorphization/partitioning.
This removes a few uses of dynamic dispatch and instead uses generics, as well as an enum to allow for other partitioning methods to be added later.
Add `rustc_fluent_macro` to decouple fluent from `rustc_macros`
Fluent, with all the icu4x it brings in, takes quite some time to compile. `fluent_messages!` is only needed in further downstream rustc crates, but is blocking more upstream crates like `rustc_index`. By splitting it out, we allow `rustc_macros` to be compiled earlier, which speeds up `x check compiler` by about 5 seconds (and even more after the needless dependency on `serde_json` is removed from `rustc_data_structures`).
Fluent, with all the icu4x it brings in, takes quite some time to
compile. `fluent_messages!` is only needed in further downstream rustc
crates, but is blocking more upstream crates like `rustc_index`. By
splitting it out, we allow `rustc_macros` to be compiled earlier, which
speeds up `x check compiler` by about 5 seconds (and even more after the
needless dependency on `serde_json` is removed from
`rustc_data_structures`).
Unify terminology used in unwind action and terminator, and reflect
the fact that a nounwind panic is triggered instead of an immediate
abort is triggered for this terminator.
