mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #94305 - JakobDegen:dp-1, r=oli-obk

Browse Source 
Remove an unnecessary restriction in `dest_prop`

I had asked about this [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Do.20unions.20have.20active.20fields.3F) but didn't receive a response, so putting up this PR that makes the change I think we can. If it turns out that this is wrong, hopefully I'll find out here. Reposting my Zulip comment:
> Not sure what channel to put this into, so using this as a fallback. The dest prop MIR opt has this comment:
>
> ```rust
> //!   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.
> ```
>
> In particular, the claim seems to be that we can't take
> ```
> x = ();
> y.field = x;
> ```
> where `y` is a union having `field: ()` as one of its variants, and optimize the entire thing away (assuming `x` is unused otherwise). As far as I know though, Rust unions don't have active fields. Is this comment correct and am I missing something? Is there a worry about this interacting poorly with FFI code/C unions/LTO or something?

This PR just removes that comment and the associated code. Also it fixes one unrelated comment that did not match the code it was commenting on.

r? rust-lang/mir-opt
This commit is contained in:
Matthias Krüger 2022-02-25 14:14:38 +01:00 committed by GitHub
commit f9f97b661a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 20 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
compiler/rustc_mir_transform/src/dest_prop.rs
View File

@ -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.
@ -103,7 +97,6 @@
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::{
@ -135,7 +128,7 @@ fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
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;
@ -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),
@ -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>,
@ -845,10 +836,11 @@ fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
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)
{
@ -862,22 +854,11 @@ fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
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 {

18
src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff
View File

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

2
src/test/mir-opt/dest-prop/union.rs
View File

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