Remove dereferencing of Box from codegen by beepster4096 · Pull Request #95576 · rust-lang/rust

beepster4096 · 2022-04-01T22:36:59Z

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.

rust-highfive · 2022-04-01T22:37:03Z

r? @estebank

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

compiler/rustc_mir_transform/src/elaborate_box_derefs.rs

JakobDegen · 2022-04-01T22:46:53Z

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

beepster4096 · 2022-04-02T01:56:43Z

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

beepster4096 · 2022-04-02T02:02:06Z

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

beepster4096 · 2022-04-02T05:05:34Z

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

beepster4096 · 2022-04-02T05:14:16Z

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

src/doc/unstable-book/src/language-features/lang-items.md

estebank · 2022-04-05T16:23:01Z

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

bors · 2022-04-07T00:13:36Z

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

michaelwoerister · 2022-04-07T08:56:01Z

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 · 2022-04-07T11:46:06Z

r? @oli-obk

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

oli-obk · 2022-04-07T12:03:13Z

@bors try @rust-timer queue

rust-timer · 2022-04-07T12:03:14Z

Awaiting bors try build completion.

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

bors · 2022-04-07T12:03:20Z

🔒 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:

git checkout box_erasure (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
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.

oli-obk

This is great! thanks for doing it.

compiler/rustc_mir_dataflow/src/elaborate_drops.rs

compiler/rustc_mir_transform/src/elaborate_box_derefs.rs

oli-obk · 2022-04-14T08:33:12Z

@bors try @rust-timer queue

rust-timer · 2022-04-14T08:33:14Z

Awaiting bors try build completion.

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

oli-obk · 2022-06-21T09:54:48Z

@bors r+

bors · 2022-06-21T09:54:50Z

📌 Commit 28ff0df has been approved by oli-obk

bors · 2022-06-21T11:00:42Z

⌛ Testing commit 28ff0df with merge a25b131...

bors · 2022-06-21T13:41:25Z

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

rust-timer · 2022-06-21T15:03:11Z

Finished benchmarking commit (a25b131): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
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

	mean¹	max	count²
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

	mean¹	max	count²
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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

oli-obk · 2022-06-21T15:15:06Z

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.

CAD97 · 2022-06-22T19:24:48Z

compiler/rustc_mir_transform/src/elaborate_box_derefs.rs

+            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")


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 · 2022-06-28T15:51:41Z

@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?

safinaskar · 2025-02-06T01:27:46Z

@rustbot label A-box

safinaskar · 2025-02-07T08:14:54Z

@rustbot label A-box

rust-highfive assigned estebank Apr 1, 2022

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 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2022

JakobDegen reviewed Apr 1, 2022

View reviewed changes

compiler/rustc_mir_transform/src/elaborate_box_derefs.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bjorn3 reviewed Apr 2, 2022

View reviewed changes

src/doc/unstable-book/src/language-features/lang-items.md Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rust-highfive assigned michaelwoerister and unassigned estebank Apr 5, 2022

rust-highfive assigned oli-obk and unassigned michaelwoerister Apr 7, 2022

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

oli-obk requested changes Apr 7, 2022

View reviewed changes

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

beepster4096 added 9 commits June 15, 2022 18:36

add From impls for BitSet and GrowableBitSet

3e9d3d9

remove box derefs from codgen

cb41788

fix inline_into_box_place test

954fbe3

fix typos & docs

e0b0fb0

correct mirphase docs

6cb38fb

fix codegen assertion

1d1ff36

make sure miri ices when dereferencing a box

dff1f9f

condense duplicate code into seperate functions

6003c25

fix box derefs in var debug info

28ff0df

beepster4096 force-pushed the box_erasure branch from 2667b9d to 28ff0df Compare June 16, 2022 01:39

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2022

bors merged commit a25b131 into rust-lang:master Jun 21, 2022

rustbot added this to the 1.63.0 milestone Jun 21, 2022

bors mentioned this pull request Jun 21, 2022

Pull Derefer before ElaborateDrops #98145

Merged

matthiaskrgr mentioned this pull request Jun 22, 2022

ICE: expected Box to contain Unique #98372

Closed

CAD97 reviewed Jun 22, 2022

View reviewed changes

RalfJung mentioned this pull request Jun 25, 2022

reject deref'ing Box deeper in const-eval interpreter #98510

Closed

rustbot added the A-box Area: Our favorite opsem complication label Feb 6, 2025

Uh oh!

Conversation

beepster4096 commented Apr 1, 2022

Uh oh!

rust-highfive commented Apr 1, 2022

Uh oh!

Uh oh!

JakobDegen commented Apr 1, 2022

Uh oh!

This comment has been minimized.

beepster4096 commented Apr 2, 2022

Uh oh!

beepster4096 commented Apr 2, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

beepster4096 commented Apr 2, 2022

Uh oh!

beepster4096 commented Apr 2, 2022

Uh oh!

Uh oh!

This comment has been minimized.

estebank commented Apr 5, 2022

Uh oh!

bors commented Apr 7, 2022

Uh oh!

michaelwoerister commented Apr 7, 2022

Uh oh!

oli-obk commented Apr 7, 2022

Uh oh!

oli-obk commented Apr 7, 2022

Uh oh!

rust-timer commented Apr 7, 2022

Uh oh!

bors commented Apr 7, 2022

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oli-obk commented Apr 14, 2022

Uh oh!

rust-timer commented Apr 14, 2022

Uh oh!

oli-obk commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

rust-timer commented Jun 21, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

oli-obk commented Jun 21, 2022

Uh oh!

CAD97 Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

rylev commented Jun 28, 2022

Uh oh!

safinaskar commented Feb 6, 2025

Uh oh!

safinaskar commented Feb 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants