Skip to content

Remove an unnecessary restriction in dest_prop #94305

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 25, 2022
Merged

Remove an unnecessary restriction in dest_prop #94305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 7 additions & 26 deletions compiler/rustc_mir_transform/src/dest_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@
//! It must also not contain any indexing projections, since those take an arbitrary `Local` as
//! the index, and that local might only be initialized shortly before `dest` is used.
//!
//! Subtle case: If `dest` is a, or projects through a union, then we have to make sure that there
//! remains an assignment to it, since that sets the "active field" of the union. But if `src` is
//! a ZST, it might not be initialized, so there might not be any use of it before the assignment,
//! and performing the optimization would simply delete the assignment, leaving `dest`
//! uninitialized.
//!
//! * `src` must be a bare `Local` without any indirections or field projections (FIXME: Is this a
//! fundamental restriction or just current impl state?). It can be copied or moved by the
//! assignment.
Expand Down Expand Up @@ -103,7 +97,6 @@ use rustc_index::{
bit_set::{BitMatrix, BitSet},
vec::IndexVec,
};
use rustc_middle::mir::tcx::PlaceTy;
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::{dump_mir, PassWhere};
use rustc_middle::mir::{
Expand Down Expand Up @@ -135,7 +128,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let def_id = body.source.def_id();

let candidates = find_candidates(tcx, body);
let candidates = find_candidates(body);
if candidates.is_empty() {
debug!("{:?}: no dest prop candidates, done", def_id);
return;
Expand Down Expand Up @@ -803,9 +796,8 @@ struct CandidateAssignment<'tcx> {
/// comment) and also throw out assignments that involve a local that has its address taken or is
/// otherwise ineligible (eg. locals used as array indices are ignored because we cannot propagate
/// arbitrary places into array indices).
fn find_candidates<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> Vec<CandidateAssignment<'tcx>> {
fn find_candidates<'tcx>(body: &Body<'tcx>) -> Vec<CandidateAssignment<'tcx>> {
let mut visitor = FindAssignments {
tcx,
body,
candidates: Vec::new(),
ever_borrowed_locals: ever_borrowed_locals(body),
Expand All @@ -816,7 +808,6 @@ fn find_candidates<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> Vec<CandidateA
}

struct FindAssignments<'a, 'tcx> {
tcx: TyCtxt<'tcx>,
body: &'a Body<'tcx>,
candidates: Vec<CandidateAssignment<'tcx>>,
ever_borrowed_locals: BitSet<Local>,
Expand Down Expand Up @@ -845,10 +836,11 @@ impl<'tcx> Visitor<'tcx> for FindAssignments<'_, 'tcx> {
return;
}

// Can't optimize if both locals ever have their address taken (can introduce
// aliasing).
// FIXME: This can be smarter and take `StorageDead` into account (which
// invalidates borrows).
// Can't optimize if either local ever has their address taken. This optimization does
// liveness analysis only based on assignments, and a local can be live even if its
// never assigned to again, because a reference to it might be live.
// FIXME: This can be smarter and take `StorageDead` into account (which invalidates
// borrows).
if self.ever_borrowed_locals.contains(dest.local)
|| self.ever_borrowed_locals.contains(src.local)
{
Expand All @@ -862,22 +854,11 @@ impl<'tcx> Visitor<'tcx> for FindAssignments<'_, 'tcx> {
return;
}

// Handle the "subtle case" described above by rejecting any `dest` that is or
// projects through a union.
let mut place_ty = PlaceTy::from_ty(self.body.local_decls[dest.local].ty);
if place_ty.ty.is_union() {
return;
}
for elem in dest.projection {
if let PlaceElem::Index(_) = elem {
// `dest` contains an indexing projection.
return;
}

place_ty = place_ty.projection_ty(self.tcx, elem);
if place_ty.ty.is_union() {
return;
}
}

self.candidates.push(CandidateAssignment {
Expand Down
18 changes: 12 additions & 6 deletions src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,29 @@
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/union.rs:13:9: 13:11
StorageLive(_2); // scope 0 at $DIR/union.rs:13:23: 13:28
_2 = val() -> bb1; // scope 0 at $DIR/union.rs:13:23: 13:28
- StorageLive(_1); // scope 0 at $DIR/union.rs:13:9: 13:11
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that we're now not marking the _1 local as live anymore, but using it right below. Maybe that is what would be causing miri to appear to care about unions being not assigned to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't catch this when I saw the test. Yeah, that seems like a bug. Let me try and see if there's an easy fix for this

- StorageLive(_2); // scope 0 at $DIR/union.rs:13:23: 13:28
- _2 = val() -> bb1; // scope 0 at $DIR/union.rs:13:23: 13:28
+ nop; // scope 0 at $DIR/union.rs:13:9: 13:11
+ nop; // scope 0 at $DIR/union.rs:13:23: 13:28
+ (_1.0: u32) = val() -> bb1; // scope 0 at $DIR/union.rs:13:23: 13:28
// mir::Constant
// + span: $DIR/union.rs:13:23: 13:26
// + literal: Const { ty: fn() -> u32 {val}, val: Value(Scalar(<ZST>)) }
}

bb1: {
(_1.0: u32) = move _2; // scope 0 at $DIR/union.rs:13:14: 13:30
StorageDead(_2); // scope 0 at $DIR/union.rs:13:29: 13:30
- (_1.0: u32) = move _2; // scope 0 at $DIR/union.rs:13:14: 13:30
- StorageDead(_2); // scope 0 at $DIR/union.rs:13:29: 13:30
+ nop; // scope 0 at $DIR/union.rs:13:14: 13:30
+ nop; // scope 0 at $DIR/union.rs:13:29: 13:30
StorageLive(_3); // scope 1 at $DIR/union.rs:15:5: 15:27
StorageLive(_4); // scope 1 at $DIR/union.rs:15:10: 15:26
_4 = (_1.0: u32); // scope 2 at $DIR/union.rs:15:19: 15:24
StorageDead(_4); // scope 1 at $DIR/union.rs:15:26: 15:27
StorageDead(_3); // scope 1 at $DIR/union.rs:15:27: 15:28
StorageDead(_1); // scope 0 at $DIR/union.rs:16:1: 16:2
- StorageDead(_1); // scope 0 at $DIR/union.rs:16:1: 16:2
+ nop; // scope 0 at $DIR/union.rs:16:1: 16:2
return; // scope 0 at $DIR/union.rs:16:2: 16:2
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/dest-prop/union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//! Tests that projections through unions cancel `DestinationPropagation`.
//! Tests that we can propogate into places that are projections into unions
// compile-flags: -Zunsound-mir-opts
fn val() -> u32 {
1
Expand Down