-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Store a DefId
instead of an AdtDef
in AggregateKind::Adt
#92203
Conversation
The `AggregateKind` enum ends up in the final mir `Body`. Currently, any changes to `AdtDef` (regardless of how significant they are) will legitimately cause the overall result of `optimized_mir` to change, invalidating any codegen re-use involving that mir. This will get worse once we start hashing the `Span` inside `FieldDef` (which is itself contained in `AdtDef`). To try to reduce these kinds of invalidations, this commit changes `AggregateKind::Adt` to store just the `DefId`, instead of the full `AdtDef`. This allows the result of `optimized_mir` to be unchanged if the `AdtDef` changes in a way that doesn't actually affect any of the MIR we build.
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit cac431b with merge f525b58008e58fa0b4809a7102f4ebd2e14c1047... |
☀️ Try build successful - checks-actions |
Queued f525b58008e58fa0b4809a7102f4ebd2e14c1047 with parent e100ec5, future comparison URL. |
Finished benchmarking commit (f525b58008e58fa0b4809a7102f4ebd2e14c1047): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
@bors r+ rollup I think we should try and follow up on this by attempting to reduce the number of |
📌 Commit cac431b has been approved by |
@bors r- Ah what I forgot: is it now possible to write incremental tests that show that this causes fewer recompiles of optimized_mir? |
r=me with an answer to that |
I thought so, thanks for confirming @bors r+ |
📌 Commit cac431b has been approved by |
…askrgr Rollup of 5 pull requests Successful merges: - rust-lang#90625 (Add `UnwindSafe` to `Once`) - rust-lang#92121 (disable test with self-referential generator on Miri) - rust-lang#92166 (Fixed a small typo in ui test comments) - rust-lang#92203 (Store a `DefId` instead of an `AdtDef` in `AggregateKind::Adt`) - rust-lang#92231 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The
AggregateKind
enum ends up in the final mirBody
. Currently,any changes to
AdtDef
(regardless of how significant they are)will legitimately cause the overall result of
optimized_mir
to change,invalidating any codegen re-use involving that mir.
This will get worse once we start hashing the
Span
insideFieldDef
(which is itself contained in
AdtDef
).To try to reduce these kinds of invalidations, this commit changes
AggregateKind::Adt
to store just theDefId
, instead of the fullAdtDef
. This allows the result ofoptimized_mir
to be unchangedif the
AdtDef
changes in a way that doesn't actually affect anyof the MIR we build.