Skip to content
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

Remove dereferencing of Box from codegen #95576

Merged
merged 9 commits into from
Jun 21, 2022

Conversation

beepster4096
Copy link
Contributor

Through #94043, #94414, #94873, and #95328, I've been fixing issues caused by Box being treated like a pointer when it is not a pointer. However, these PRs just introduced special cases for Box. This PR removes those special cases and instead transforms a deref of Box into a deref of the pointer it contains.

Hopefully, this is the end of the Box<T, A> ICEs.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 1, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2022
@JakobDegen
Copy link
Contributor

Could you update the documentation for MirPhase to mention this change as well?

@rust-log-analyzer

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

cc @JakobDegen (you asked), @bjorn3 & @antoyo (you'll want to update your codegens after this)

@beepster4096
Copy link
Contributor Author

Oh wait lol, I didn't notice you were already here

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

It looks like this is impacting codegen, I'll have to investigate.

@beepster4096
Copy link
Contributor Author

The llvm ir only seems to be different with no optimizations, how bad are these changes then?

@rust-log-analyzer

This comment has been minimized.

@estebank
Copy link
Contributor

estebank commented Apr 5, 2022

r? rust-lang/compiler rerrolling the review dice, I'm currently a bit swamped

@bors
Copy link
Contributor

bors commented Apr 7, 2022

☔ The latest upstream changes (presumably #95742) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister
Copy link
Member

I don't think I have the required background knowledge to review this. Maybe someone from @rust-lang/wg-mir-opt could take a look?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 7, 2022

r? @oli-obk

I already reviewed the other PRs and asked for a general solution there ^^

@oli-obk
Copy link
Contributor

oli-obk commented Apr 7, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 7, 2022
@bors
Copy link
Contributor

bors commented Apr 7, 2022

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout box_erasure (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self box_erasure --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/test/mir-opt/inline/issue_58867_inline_as_ref_as_mut.d.Inline.after.mir
CONFLICT (content): Merge conflict in src/test/mir-opt/inline/issue_58867_inline_as_ref_as_mut.d.Inline.after.mir
Auto-merging src/test/mir-opt/inline/issue_58867_inline_as_ref_as_mut.b.Inline.after.mir
CONFLICT (content): Merge conflict in src/test/mir-opt/inline/issue_58867_inline_as_ref_as_mut.b.Inline.after.mir
Auto-merging compiler/rustc_mir_transform/src/lib.rs
Auto-merging compiler/rustc_mir_dataflow/src/elaborate_drops.rs
Auto-merging compiler/rustc_middle/src/mir/mod.rs
Auto-merging compiler/rustc_const_eval/src/transform/validate.rs
Auto-merging compiler/rustc_codegen_ssa/src/mir/place.rs
Auto-merging compiler/rustc_codegen_ssa/src/mir/operand.rs
Automatic merge failed; fix conflicts and then commit the result.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! thanks for doing it.

compiler/rustc_mir_dataflow/src/elaborate_drops.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_transform/src/elaborate_box_derefs.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_transform/src/elaborate_box_derefs.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Apr 14, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Jun 14, 2022

☔ The latest upstream changes (presumably #96591) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jun 21, 2022

📌 Commit 28ff0df has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2022
@bors
Copy link
Contributor

bors commented Jun 21, 2022

⌛ Testing commit 28ff0df with merge a25b131...

@bors
Copy link
Contributor

bors commented Jun 21, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a25b131 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2022
@bors bors merged commit a25b131 into rust-lang:master Jun 21, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 21, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a25b131): comparison url.

Instruction count

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
0.6% 1.6% 5
Regressions 😿
(secondary)
0.5% 0.8% 17
Improvements 🎉
(primary)
-0.5% -0.5% 1
Improvements 🎉
(secondary)
-1.2% -1.7% 8
All 😿🎉 (primary) 0.4% 1.6% 6

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
3.3% 4.8% 2
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.0% -2.0% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
2.1% 2.1% 1
Regressions 😿
(secondary)
1.7% 1.7% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.1% 2.1% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2022

the diesel regression:

161,388,984  ???:<rustc_data_structures::obligation_forest::ObligationForest<rustc_trait_selection::traits::fulfill::PendingPredicateObligation>>::process_obligations::<rustc_trait_selection::traits::fulfill::FulfillProcessor, rustc_data_structures::obligation_forest::Outcome<rustc_trait_selection::traits::fulfill::PendingPredicateObligation, rustc_infer::traits::FulfillmentErrorCode>>
-47,232,325  ???:rustc_trait_selection::traits::project::opt_normalize_projection_type
 34,318,965  ???:<alloc::vec::drain_filter::DrainFilter<rustc_infer::traits::Obligation<rustc_middle::ty::Predicate>, rustc_trait_selection::traits::project::opt_normalize_projection_type::{closure
-22,131,465  ???:<alloc::rc::Rc<rustc_middle::traits::ObligationCauseCode> as core::ops::drop::Drop>::drop
 16,443,992  ???:<rustc_infer::infer::InferCtxt>::commit_if_ok::<(), (), rustc_trait_selection::traits::project::assemble_candidates_from_impls::{closure
  6,792,050  ???:core::ptr::drop_in_place::<rustc_middle::traits::ObligationCauseCode>
  5,680,738  ???:<rustc_trait_selection::traits::fulfill::FulfillProcessor as rustc_data_structures::obligation_forest::ObligationProcessor>::process_obligation
  5,261,136  ???:<rustc_infer::infer::canonical::canonicalizer::Canonicalizer as rustc_middle::ty::fold::FallibleTypeFolder>::try_fold_predicate
 -4,554,398  ???:<rustc_infer::infer::InferCtxt as rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt>::predicate_may_hold

the keccak regression is

161,388,984  ???:<rustc_data_structures::obligation_forest::ObligationForest<rustc_trait_selection::traits::fulfill::PendingPredicateObligation>>::process_obligations::<rustc_trait_selection::traits::fulfill::FulfillProcessor, rustc_data_structures::obligation_forest::Outcome<rustc_trait_selection::traits::fulfill::PendingPredicateObligation, rustc_infer::traits::FulfillmentErrorCode>>
-47,232,325  ???:rustc_trait_selection::traits::project::opt_normalize_projection_type
 34,318,965  ???:<alloc::vec::drain_filter::DrainFilter<rustc_infer::traits::Obligation<rustc_middle::ty::Predicate>, rustc_trait_selection::traits::project::opt_normalize_projection_type::{closure
-22,131,465  ???:<alloc::rc::Rc<rustc_middle::traits::ObligationCauseCode> as core::ops::drop::Drop>::drop
 16,443,992  ???:<rustc_infer::infer::InferCtxt>::commit_if_ok::<(), (), rustc_trait_selection::traits::project::assemble_candidates_from_impls::{closure
  6,792,050  ???:core::ptr::drop_in_place::<rustc_middle::traits::ObligationCauseCode>
  5,680,738  ???:<rustc_trait_selection::traits::fulfill::FulfillProcessor as rustc_data_structures::obligation_forest::ObligationProcessor>::process_obligation
  5,261,136  ???:<rustc_infer::infer::canonical::canonicalizer::Canonicalizer as rustc_middle::ty::fold::FallibleTypeFolder>::try_fold_predicate
 -4,554,398  ???:<rustc_infer::infer::InferCtxt as rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt>::predicate_may_hold
 -2,848,486  ???:<rustc_middle::ty::walk::TypeWalker as core::iter::traits::iterator::Iterator>::next
 -2,766,969  ???:<rustc_data_structures::snapshot_map::SnapshotMap<rustc_infer::traits::project::ProjectionCacheKey, rustc_infer::traits::project::ProjectionCacheEntry, &mut std::collections::hash::map::HashMap<rustc_infer::traits::project::ProjectionCacheKey, rustc_infer::traits::project::ProjectionCacheEntry, core::hash::BuildHasherDefault<rustc_hash::FxHasher>>, &mut rustc_infer::infer::undo_log::InferCtxtUndoLogs>>::insert
  2,721,476  ???:<rustc_infer::infer::InferCtxt>::probe::<core::result::Result<rustc_middle::traits::select::EvaluationResult, rustc_middle::traits::select::OverflowError>, rustc_trait_selection::traits::relationships::update<rustc_trait_selection::traits::fulfill::FulfillmentContext>::{closure
  2,207,575  ???:<rustc_infer::traits::project::ProjectionCache>::ambiguous
 -2,062,204  ???:<rustc_trait_selection::traits::fulfill::FulfillmentContext as rustc_infer::traits::engine::TraitEngine>::register_predicate_obligation

So it looks like we're doing a bit more obligation processing now.

let unique_did = tcx.adt_def(def_id).non_enum_variant().fields[0].did;

let Some(nonnull_def) = tcx.type_of(unique_did).ty_adt_def() else {
span_bug!(tcx.def_span(unique_did), "expected Box to contain Unique")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be using a lang item for Unique rather than just expecting it to be right. Perhaps even #[lang = "owned_box"] could wf check that the first field is Unique.

Reported as an ICE by #98372; rustc_codegen_gcc's minicore defines Box as (*mut T, A).

@rylev
Copy link
Member

rylev commented Jun 28, 2022

@oli-obk this regression is only on average a 0.4% regression across only 6 cases which doesn't seem like a big deal. Would you say this change is worth the performance penalty? Shall we mark as triaged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.