From 2cf45bc9346cdd43fe93aa72deed422024020820 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 23 Jan 2020 10:59:25 +0900 Subject: [PATCH 1/8] Fix SimplifyArmIdentity not working with temporaries and storage markers --- src/librustc_mir/transform/simplify_try.rs | 193 +++++++++++++++++---- 1 file changed, 160 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index e733b0a5b5928..1bb7fbbdeeca6 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -13,6 +13,7 @@ use crate::transform::{simplify, MirPass, MirSource}; use itertools::Itertools as _; use rustc::mir::*; use rustc::ty::{Ty, TyCtxt}; +use rustc_index::vec::IndexVec; use rustc_target::abi::VariantIdx; /// Simplifies arms of form `Variant(x) => Variant(x)` to just a move. @@ -25,6 +26,19 @@ use rustc_target::abi::VariantIdx; /// discriminant(_LOCAL_0) = VAR_IDX; /// ``` /// +/// or +/// +/// ```rust +/// StorageLive(_LOCAL_TMP); +/// _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); +/// StorageLive(_LOCAL_TMP_2); +/// _LOCAL_TMP_2 = _LOCAL_TMP +/// ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP_2; +/// discriminant(_LOCAL_0) = VAR_IDX; +/// StorageDead(_LOCAL_TMP_2); +/// StorageDead(_LOCAL_TMP); +/// ``` +/// /// into: /// /// ```rust @@ -36,44 +50,157 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity { fn run_pass(&self, _: TyCtxt<'tcx>, _: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>) { let (basic_blocks, local_decls) = body.basic_blocks_and_local_decls_mut(); for bb in basic_blocks { - // Need 3 statements: - let (s0, s1, s2) = match &mut *bb.statements { - [s0, s1, s2] => (s0, s1, s2), - _ => continue, - }; - - // Pattern match on the form we want: - let (local_tmp_s0, local_1, vf_s0) = match match_get_variant_field(s0) { - None => continue, - Some(x) => x, - }; - let (local_tmp_s1, local_0, vf_s1) = match match_set_variant_field(s1) { - None => continue, - Some(x) => x, - }; - if local_tmp_s0 != local_tmp_s1 - // The field-and-variant information match up. - || vf_s0 != vf_s1 - // Source and target locals have the same type. - // FIXME(Centril | oli-obk): possibly relax to same layout? - || local_decls[local_0].ty != local_decls[local_1].ty - // We're setting the discriminant of `local_0` to this variant. - || Some((local_0, vf_s0.var_idx)) != match_set_discr(s2) - { - continue; + match &mut *bb.statements { + [s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls), + other => match_arm(other, local_decls), } + } + } +} + +/// Match on: +/// ```rust +/// _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); +/// ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP; +/// discriminant(_LOCAL_0) = VAR_IDX; +/// ``` +fn match_copypropd_arm( + [s0, s1, s2]: [&mut Statement<'_>; 3], + local_decls: &mut IndexVec>, +) { + // Pattern match on the form we want: + let (local_tmp_s0, local_1, vf_s0) = match match_get_variant_field(s0) { + None => return, + Some(x) => x, + }; + let (local_tmp_s1, local_0, vf_s1) = match match_set_variant_field(s1) { + None => return, + Some(x) => x, + }; + if local_tmp_s0 != local_tmp_s1 + // The field-and-variant information match up. + || vf_s0 != vf_s1 + // Source and target locals have the same type. + // FIXME(Centril | oli-obk): possibly relax to same layout? + || local_decls[local_0].ty != local_decls[local_1].ty + // We're setting the discriminant of `local_0` to this variant. + || Some((local_0, vf_s0.var_idx)) != match_set_discr(s2) + { + return; + } + + // Right shape; transform! + match &mut s0.kind { + StatementKind::Assign(box (place, rvalue)) => { + *place = local_0.into(); + *rvalue = Rvalue::Use(Operand::Move(local_1.into())); + } + _ => unreachable!(), + } + s1.make_nop(); + s2.make_nop(); +} - // Right shape; transform! - match &mut s0.kind { - StatementKind::Assign(box (place, rvalue)) => { - *place = local_0.into(); - *rvalue = Rvalue::Use(Operand::Move(local_1.into())); +/// Match on: +/// ```rust +/// StorageLive(_LOCAL_TMP); +/// _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); +/// StorageLive(_LOCAL_TMP_2); +/// _LOCAL_TMP_2 = _LOCAL_TMP +/// ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP_2; +/// discriminant(_LOCAL_0) = VAR_IDX; +/// StorageDead(_LOCAL_TMP_2); +/// StorageDead(_LOCAL_TMP); +/// ``` +fn match_arm(stmts: &mut [Statement<'_>], local_decls: &mut IndexVec>) { + if stmts.len() != 8 { + return; + } + + // StorageLive(_LOCAL_TMP); + let local_tmp_live = if let StatementKind::StorageLive(local_tmp_live) = &stmts[0].kind { + *local_tmp_live + } else { + return; + }; + + // _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); + let (local_tmp, local_1, vf_1) = match match_get_variant_field(&stmts[1]) { + None => return, + Some(x) => x, + }; + + if local_tmp_live != local_tmp { + return; + } + + // StorageLive(_LOCAL_TMP_2); + let local_tmp_2_live = if let StatementKind::StorageLive(local_tmp_2_live) = &stmts[2].kind { + *local_tmp_2_live + } else { + return; + }; + + // _LOCAL_TMP_2 = _LOCAL_TMP + if let StatementKind::Assign(box (lhs, Rvalue::Use(rhs))) = &stmts[3].kind { + let lhs = lhs.as_local(); + if lhs != Some(local_tmp_2_live) { + return; + } + match rhs { + Operand::Copy(rhs) | Operand::Move(rhs) => { + if rhs.as_local() != Some(local_tmp) { + return; } - _ => unreachable!(), } - s1.make_nop(); - s2.make_nop(); + _ => return, } + } else { + return; + } + + // ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP_2; + let (local_tmp_2, local_0, vf_0) = match match_set_variant_field(&stmts[4]) { + None => return, + Some(x) => x, + }; + + if local_tmp_2 != local_tmp_2_live { + return; + } + + if vf_1 != vf_0 // The field-and-variant information match up. + // Source and target locals have the same type. + // FIXME(Centril | oli-obk): possibly relax to same layout? + || local_decls[local_0].ty != local_decls[local_1].ty + // We're setting the discriminant of `local_0` to this variant. + || Some((local_0, vf_1.var_idx)) != match_set_discr(&stmts[5]) + { + return; + } + + // StorageDead(_LOCAL_TMP_2); + // StorageDead(_LOCAL_TMP); + match (&stmts[6].kind, &stmts[7].kind) { + ( + StatementKind::StorageDead(local_tmp_2_dead), + StatementKind::StorageDead(local_tmp_dead), + ) => { + if *local_tmp_2_dead != local_tmp_2 || *local_tmp_dead != local_tmp { + return; + } + } + _ => return, + } + + // Right shape; transform! + stmts[0].kind = StatementKind::Assign(Box::new(( + local_0.into(), + Rvalue::Use(Operand::Move(local_1.into())), + ))); + + for s in &mut stmts[1..] { + s.make_nop(); } } From d0a9500b7d1f7e292481097dd40dfca69b2fb9dc Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 23 Jan 2020 11:29:14 +0900 Subject: [PATCH 2/8] Add test --- src/test/mir-opt/simplify_try_opt_1.rs | 118 +++++++++++++++++++++++++ src/tools/compiletest/src/runtest.rs | 10 +-- 2 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 src/test/mir-opt/simplify_try_opt_1.rs diff --git a/src/test/mir-opt/simplify_try_opt_1.rs b/src/test/mir-opt/simplify_try_opt_1.rs new file mode 100644 index 0000000000000..0913bc3ae9cc0 --- /dev/null +++ b/src/test/mir-opt/simplify_try_opt_1.rs @@ -0,0 +1,118 @@ +// ignore-tidy-linelength +// compile-flags: -Zmir-opt-level=1 + +// Note: Applying the optimization to `?` requires MIR inlining, +// which is not run in `mir-opt-level=1`. + +fn try_identity(x: Result) -> Result { + match x { + Ok(x) => Ok(x), + Err(x) => Err(x), + } +} + +fn main() { + let _ = try_identity(Ok(0)); +} + +// END RUST SOURCE +// START rustc.try_identity.SimplifyArmIdentity.before.mir +// fn try_identity(_1: std::result::Result) -> std::result::Result { +// ... +// bb0: { +// _2 = discriminant(_1); +// switchInt(move _2) -> [0isize: bb2, 1isize: bb3, otherwise: bb1]; +// } +// bb1: { +// unreachable; +// } +// bb2: { +// StorageLive(_3); +// _3 = ((_1 as Ok).0: u64); +// StorageLive(_4); +// _4 = _3; +// ((_0 as Ok).0: u64) = move _4; +// discriminant(_0) = 0; +// StorageDead(_4); +// StorageDead(_3); +// goto -> bb4; +// } +// bb3: { +// StorageLive(_5); +// _5 = ((_1 as Err).0: i32); +// StorageLive(_6); +// _6 = _5; +// ((_0 as Err).0: i32) = move _6; +// discriminant(_0) = 1; +// StorageDead(_6); +// StorageDead(_5); +// goto -> bb4; +// } +// bb4: { +// return; +// } +// } +// END rustc.try_identity.SimplifyArmIdentity.before.mir + + +// START rustc.try_identity.SimplifyArmIdentity.after.mir +// fn try_identity(_1: std::result::Result) -> std::result::Result { +// ... +// bb0: { +// _2 = discriminant(_1); +// switchInt(move _2) -> [0isize: bb2, 1isize: bb3, otherwise: bb1]; +// } +// bb1: { +// unreachable; +// } +// bb2: { +// _0 = move _1; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// goto -> bb4; +// } +// bb3: { +// _0 = move _1; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// goto -> bb4; +// } +// bb4: { +// return; +// } +// } +// END rustc.try_identity.SimplifyArmIdentity.after.mir + +// START rustc.try_identity.SimplifyBranchSame.after.mir +// fn try_identity(_1: std::result::Result) -> std::result::Result { +// ... +// bb0: { +// _2 = discriminant(_1); +// goto -> bb1; +// } +// bb1: { +// _0 = move _1; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// goto -> bb2; +// } +// bb2: { +// return; +// } +// } +// END rustc.try_identity.SimplifyBranchSame.after.mir diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 3a114a0b71517..1279c83d2242d 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -1872,11 +1872,11 @@ impl<'test> TestCx<'test> { rustc.arg("-Zdeduplicate-diagnostics=no"); } MirOpt => { - rustc.args(&[ - "-Zdump-mir=all", - "-Zmir-opt-level=3", - "-Zdump-mir-exclude-pass-number", - ]); + if !self.props.compile_flags.iter().any(|s| s.starts_with("-Zmir-opt-level")) { + rustc.arg("-Zmir-opt-level=3"); + } + + rustc.args(&["-Zdump-mir=all", "-Zdump-mir-exclude-pass-number"]); let mir_dump_dir = self.get_mir_dump_dir(); let _ = fs::remove_dir_all(&mir_dump_dir); From bde6308e32f2571b7098ab3b08485dcc42bb2c5d Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 23 Jan 2020 14:56:15 +0900 Subject: [PATCH 3/8] Add SinkCommonCodeFromPredecessors pass --- src/librustc_mir/transform/mod.rs | 1 + src/librustc_mir/transform/simplify_try.rs | 126 ++++++++++++++--- src/test/mir-opt/simplify_try_opt_1.rs | 150 ++++++++++++++++++++- 3 files changed, 254 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 3c37eccc1843b..3e0059a1dcf7a 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -317,6 +317,7 @@ fn run_optimization_passes<'tcx>( &simplify::SimplifyCfg::new("after-remove-noop-landing-pads"), &simplify_try::SimplifyArmIdentity, &simplify_try::SimplifyBranchSame, + &simplify_try::SinkCommonCodeFromPredecessors, &simplify::SimplifyCfg::new("final"), &simplify::SimplifyLocals, &add_call_guards::CriticalCallEdges, diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index 1bb7fbbdeeca6..74695d74064b4 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -12,7 +12,7 @@ use crate::transform::{simplify, MirPass, MirSource}; use itertools::Itertools as _; use rustc::mir::*; -use rustc::ty::{Ty, TyCtxt}; +use rustc::ty::{ParamEnv, Ty, TyCtxt}; use rustc_index::vec::IndexVec; use rustc_target::abi::VariantIdx; @@ -47,12 +47,17 @@ use rustc_target::abi::VariantIdx; pub struct SimplifyArmIdentity; impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity { - fn run_pass(&self, _: TyCtxt<'tcx>, _: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>) { + fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>) { + let param_env = tcx.param_env(src.def_id()); let (basic_blocks, local_decls) = body.basic_blocks_and_local_decls_mut(); for bb in basic_blocks { match &mut *bb.statements { - [s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls), - other => match_arm(other, local_decls), + [.., s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls), + _ => {} + } + + if 8 <= bb.statements.len() && bb.statements.len() <= 9 { + match_arm(&mut bb.statements, local_decls, tcx, param_env); } } } @@ -104,6 +109,7 @@ fn match_copypropd_arm( /// Match on: /// ```rust /// StorageLive(_LOCAL_TMP); +/// [ _DROP_FLAG = const false; ] /// _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); /// StorageLive(_LOCAL_TMP_2); /// _LOCAL_TMP_2 = _LOCAL_TMP @@ -112,8 +118,13 @@ fn match_copypropd_arm( /// StorageDead(_LOCAL_TMP_2); /// StorageDead(_LOCAL_TMP); /// ``` -fn match_arm(stmts: &mut [Statement<'_>], local_decls: &mut IndexVec>) { - if stmts.len() != 8 { +fn match_arm<'tcx>( + stmts: &mut [Statement<'tcx>], + local_decls: &mut IndexVec>, + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, +) { + if stmts.len() < 8 || stmts.len() > 9 { return; } @@ -124,8 +135,21 @@ fn match_arm(stmts: &mut [Statement<'_>], local_decls: &mut IndexVec + { + match (place.as_local(), c.literal.try_eval_bool(tcx, param_env)) { + (Some(local), Some(false)) => (1, Some(local)), + _ => return, + } + } + _ => (0, None), + }; + // _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); - let (local_tmp, local_1, vf_1) = match match_get_variant_field(&stmts[1]) { + let (local_tmp, local_1, vf_1) = match match_get_variant_field(&stmts[idx + 1]) { None => return, Some(x) => x, }; @@ -135,14 +159,15 @@ fn match_arm(stmts: &mut [Statement<'_>], local_decls: &mut IndexVec], local_decls: &mut IndexVec return, Some(x) => x, }; @@ -169,19 +194,23 @@ fn match_arm(stmts: &mut [Statement<'_>], local_decls: &mut IndexVec], local_decls: &mut IndexVec MirPass<'tcx> for SimplifyBranchSame { } } } + +pub struct SinkCommonCodeFromPredecessors; + +impl<'tcx> MirPass<'tcx> for SinkCommonCodeFromPredecessors { + fn run_pass(&self, _tcx: TyCtxt<'tcx>, _src: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>) { + for (bb, preds) in body.predecessors().clone().iter_enumerated() { + if preds.len() < 2 + || preds + .iter() + .any(|p| body.basic_blocks()[*p].terminator().successors().count() != 1) + { + continue; + } + + let mut matched_stmts = 0; + let basic_blocks = body.basic_blocks_mut(); + + loop { + if let Some(stmt) = basic_blocks[preds[0]].statements.iter().nth_back(matched_stmts) + { + if preds[1..].iter().any(|&p| { + basic_blocks[p].statements.iter().nth_back(matched_stmts).map(|s| &s.kind) + != Some(&stmt.kind) + }) { + break; + } + } else { + break; + } + + matched_stmts += 1; + } + + if matched_stmts == 0 { + continue; + } + + for p in &preds[1..] { + let stmts = &mut basic_blocks[*p].statements; + let len = stmts.len(); + stmts.truncate(len - matched_stmts); + } + + let stmts = &mut basic_blocks[preds[0]].statements; + let mut matched_stmts = stmts.drain(stmts.len() - matched_stmts..).collect::>(); + + let stmts = &mut basic_blocks[bb].statements; + let orig = std::mem::take(stmts); + matched_stmts.extend(orig); + *stmts = matched_stmts; + } + } +} diff --git a/src/test/mir-opt/simplify_try_opt_1.rs b/src/test/mir-opt/simplify_try_opt_1.rs index 0913bc3ae9cc0..ff219ed820250 100644 --- a/src/test/mir-opt/simplify_try_opt_1.rs +++ b/src/test/mir-opt/simplify_try_opt_1.rs @@ -11,8 +11,16 @@ fn try_identity(x: Result) -> Result { } } +fn try_identity_drop(x: Result) -> Result { + match x { + Ok(x) => Ok(x), + Err(x) => Err(x), + } +} + fn main() { let _ = try_identity(Ok(0)); + let _ = try_identity_drop(Err(0)); } // END RUST SOURCE @@ -93,14 +101,97 @@ fn main() { // } // END rustc.try_identity.SimplifyArmIdentity.after.mir -// START rustc.try_identity.SimplifyBranchSame.after.mir +// START rustc.try_identity.SimplifyCfg-final.after.mir // fn try_identity(_1: std::result::Result) -> std::result::Result { // ... // bb0: { // _2 = discriminant(_1); -// goto -> bb1; +// _0 = move _1; +// return; +// } +// } +// END rustc.try_identity.SimplifyCfg-final.after.mir + +// START rustc.try_identity_drop.SimplifyArmIdentity.before.mir +// fn try_identity_drop(_1: std::result::Result) -> std::result::Result { +// ... +// bb0: { +// _7 = const false; +// _7 = const true; +// _2 = discriminant(_1); +// switchInt(move _2) -> [0isize: bb2, 1isize: bb3, otherwise: bb1]; +// } +// bb1: { +// unreachable; +// } +// bb2: { +// StorageLive(_3); +// _7 = const false; +// _3 = move ((_1 as Ok).0: std::string::String); +// StorageLive(_4); +// _4 = move _3; +// ((_0 as Ok).0: std::string::String) = move _4; +// discriminant(_0) = 0; +// StorageDead(_4); +// StorageDead(_3); +// goto -> bb8; +// } +// bb3: { +// StorageLive(_5); +// _5 = ((_1 as Err).0: i32); +// StorageLive(_6); +// _6 = _5; +// ((_0 as Err).0: i32) = move _6; +// discriminant(_0) = 1; +// StorageDead(_6); +// StorageDead(_5); +// goto -> bb8; +// } +// bb4: { +// return; +// } +// bb5: { +// switchInt(_7) -> [false: bb4, otherwise: bb6]; +// } +// bb6: { +// _7 = const false; +// drop(((_1 as Ok).0: std::string::String)) -> bb4; +// } +// bb7: { +// drop(_1) -> bb4; +// } +// bb8: { +// _8 = discriminant(_1); +// switchInt(move _8) -> [0isize: bb5, otherwise: bb7]; +// } +// } +// END rustc.try_identity_drop.SimplifyArmIdentity.before.mir + +// START rustc.try_identity_drop.SimplifyArmIdentity.after.mir +// fn try_identity_drop(_1: std::result::Result) -> std::result::Result { +// ... +// bb0: { +// _7 = const false; +// _7 = const true; +// _2 = discriminant(_1); +// switchInt(move _2) -> [0isize: bb2, 1isize: bb3, otherwise: bb1]; // } // bb1: { +// unreachable; +// } +// bb2: { +// _7 = const false; +// _0 = move _1; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// nop; +// goto -> bb8; +// } +// bb3: { // _0 = move _1; // nop; // nop; @@ -109,10 +200,61 @@ fn main() { // nop; // nop; // nop; -// goto -> bb2; +// goto -> bb8; +// } +// bb4: { +// return; +// } +// bb5: { +// switchInt(_7) -> [false: bb4, otherwise: bb6]; +// } +// bb6: { +// _7 = const false; +// drop(((_1 as Ok).0: std::string::String)) -> bb4; +// } +// bb7: { +// drop(_1) -> bb4; +// } +// bb8: { +// _8 = discriminant(_1); +// switchInt(move _8) -> [0isize: bb5, otherwise: bb7]; +// } +// } +// END rustc.try_identity_drop.SimplifyArmIdentity.after.mir + +// START rustc.try_identity_drop.SimplifyCfg-final.after.mir +// fn try_identity_drop(_1: std::result::Result) -> std::result::Result { +// ... +// bb0: { +// _7 = const false; +// _7 = const true; +// _2 = discriminant(_1); +// switchInt(move _2) -> [0isize: bb2, 1isize: bb7, otherwise: bb1]; +// } +// bb1: { +// unreachable; // } // bb2: { +// _7 = const false; +// goto -> bb7; +// } +// bb3: { // return; // } +// bb4: { +// switchInt(_7) -> [false: bb3, otherwise: bb5]; +// } +// bb5: { +// _7 = const false; +// drop(((_1 as Ok).0: std::string::String)) -> bb3; +// } +// bb6: { +// drop(_1) -> bb3; +// } +// bb7: { +// _0 = move _1; +// _8 = discriminant(_1); +// switchInt(move _8) -> [0isize: bb4, otherwise: bb6]; +// } // } -// END rustc.try_identity.SimplifyBranchSame.after.mir +// END rustc.try_identity_drop.SimplifyCfg-final.after.mir From 4183719084043d0f70fdaed69cb6c1485787d877 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 23 Jan 2020 23:39:26 +0900 Subject: [PATCH 4/8] Dedup predecessors and filter out non-goto blocks --- src/librustc_mir/transform/simplify_try.rs | 35 ++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index 74695d74064b4..de1432c29b24b 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -52,7 +52,7 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity { let (basic_blocks, local_decls) = body.basic_blocks_and_local_decls_mut(); for bb in basic_blocks { match &mut *bb.statements { - [.., s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls), + [s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls), _ => {} } @@ -141,7 +141,9 @@ fn match_arm<'tcx>( if c.literal.ty.is_bool() => { match (place.as_local(), c.literal.try_eval_bool(tcx, param_env)) { - (Some(local), Some(false)) => (1, Some(local)), + (Some(local), Some(false)) if !local_decls[local].is_user_variable() => { + (1, Some(local)) + } _ => return, } } @@ -194,9 +196,9 @@ fn match_arm<'tcx>( return; } - if drop_flag == Some(local_0) - || drop_flag == Some(local_tmp) + if drop_flag == Some(local_tmp) || drop_flag == Some(local_tmp_2) + || local_decls[local_tmp_2].is_user_variable() // The field-and-variant information match up. || vf_1 != vf_0 // Source and target locals have the same type. @@ -366,17 +368,30 @@ pub struct SinkCommonCodeFromPredecessors; impl<'tcx> MirPass<'tcx> for SinkCommonCodeFromPredecessors { fn run_pass(&self, _tcx: TyCtxt<'tcx>, _src: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>) { - for (bb, preds) in body.predecessors().clone().iter_enumerated() { + for (bb, mut preds) in body.predecessors().clone().into_iter_enumerated() { + if bb == START_BLOCK || preds.len() < 2 { + continue; + } + + let is_goto = |terminator: &Terminator<'_>| { + if let TerminatorKind::Goto { .. } = terminator.kind { true } else { false } + }; + let basic_blocks = body.basic_blocks(); + + preds.sort_unstable(); + preds.dedup(); + let is_cleanup = basic_blocks[bb].is_cleanup; if preds.len() < 2 - || preds - .iter() - .any(|p| body.basic_blocks()[*p].terminator().successors().count() != 1) + || preds.iter().any(|&p| { + p == bb + || basic_blocks[p].is_cleanup != is_cleanup + || !is_goto(basic_blocks[p].terminator()) + }) { continue; } let mut matched_stmts = 0; - let basic_blocks = body.basic_blocks_mut(); loop { if let Some(stmt) = basic_blocks[preds[0]].statements.iter().nth_back(matched_stmts) @@ -398,6 +413,8 @@ impl<'tcx> MirPass<'tcx> for SinkCommonCodeFromPredecessors { continue; } + let basic_blocks = body.basic_blocks_mut(); + for p in &preds[1..] { let stmts = &mut basic_blocks[*p].statements; let len = stmts.len(); From 4ff009bf29b3741946b17bfbd7866a01e8b70552 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 24 Jan 2020 09:26:04 +0900 Subject: [PATCH 5/8] Don't run on types with destructors --- src/librustc_mir/transform/simplify_try.rs | 17 ++++-- src/test/mir-opt/simplify_try_opt_1.rs | 62 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index de1432c29b24b..e77e1f12c2510 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -52,7 +52,7 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity { let (basic_blocks, local_decls) = body.basic_blocks_and_local_decls_mut(); for bb in basic_blocks { match &mut *bb.statements { - [s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls), + [s0, s1, s2] => match_copypropd_arm([s0, s1, s2], local_decls, tcx), _ => {} } @@ -69,9 +69,10 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity { /// ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP; /// discriminant(_LOCAL_0) = VAR_IDX; /// ``` -fn match_copypropd_arm( - [s0, s1, s2]: [&mut Statement<'_>; 3], - local_decls: &mut IndexVec>, +fn match_copypropd_arm<'tcx>( + [s0, s1, s2]: [&mut Statement<'tcx>; 3], + local_decls: &mut IndexVec>, + tcx: TyCtxt<'tcx>, ) { // Pattern match on the form we want: let (local_tmp_s0, local_1, vf_s0) = match match_get_variant_field(s0) { @@ -88,6 +89,8 @@ fn match_copypropd_arm( // Source and target locals have the same type. // FIXME(Centril | oli-obk): possibly relax to same layout? || local_decls[local_0].ty != local_decls[local_1].ty + // `match` on a value with dtor will drop the original value. + || has_dtor(tcx, local_decls[local_1].ty) // We're setting the discriminant of `local_0` to this variant. || Some((local_0, vf_s0.var_idx)) != match_set_discr(s2) { @@ -204,6 +207,8 @@ fn match_arm<'tcx>( // Source and target locals have the same type. // FIXME(Centril | oli-obk): possibly relax to same layout? || local_decls[local_0].ty != local_decls[local_1].ty + // `match` on a value with dtor will drop the original value. + || has_dtor(tcx, local_decls[local_1].ty) // We're setting the discriminant of `local_0` to this variant. || Some((local_0, vf_1.var_idx)) != match_set_discr(&stmts[idx + 5]) { @@ -241,6 +246,10 @@ fn match_arm<'tcx>( } } +fn has_dtor<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool { + ty.ty_adt_def().map(|def| def.has_dtor(tcx)).unwrap_or(false) +} + /// Match on: /// ```rust /// _LOCAL_INTO = ((_LOCAL_FROM as Variant).FIELD: TY); diff --git a/src/test/mir-opt/simplify_try_opt_1.rs b/src/test/mir-opt/simplify_try_opt_1.rs index ff219ed820250..8d5e7fc61f2af 100644 --- a/src/test/mir-opt/simplify_try_opt_1.rs +++ b/src/test/mir-opt/simplify_try_opt_1.rs @@ -18,9 +18,30 @@ fn try_identity_drop(x: Result) -> Result { } } +// Nop-match may actually have to call destructor on the original value. +// Checks that we don't optimize them. + +enum HasDtor { + Foo(i32), + Bar(u64), +} + +impl Drop for HasDtor { + fn drop(&mut self) {} +} + +fn match_dtor(x: HasDtor) -> HasDtor { + use HasDtor::*; + match x { + Foo(x) => Foo(x), + Bar(x) => Bar(x), + } +} + fn main() { let _ = try_identity(Ok(0)); let _ = try_identity_drop(Err(0)); + let _ = match_dtor(HasDtor::Foo(0)); } // END RUST SOURCE @@ -258,3 +279,44 @@ fn main() { // } // } // END rustc.try_identity_drop.SimplifyCfg-final.after.mir + +// START rustc.match_dtor.SimplifyCfg-final.after.mir +// fn match_dtor(_1: HasDtor) -> HasDtor { +// ... +// bb0: { +// _2 = discriminant(_1); +// switchInt(move _2) -> [0isize: bb2, 1isize: bb3, otherwise: bb1]; +// } +// bb1: { +// unreachable; +// } +// bb2: { +// StorageLive(_3); +// _3 = ((_1 as Foo).0: i32); +// StorageLive(_4); +// _4 = _3; +// ((_0 as Foo).0: i32) = move _4; +// discriminant(_0) = 0; +// StorageDead(_4); +// StorageDead(_3); +// goto -> bb4; +// } +// bb3: { +// StorageLive(_5); +// _5 = ((_1 as Bar).0: u64); +// StorageLive(_6); +// _6 = _5; +// ((_0 as Bar).0: u64) = move _6; +// discriminant(_0) = 1; +// StorageDead(_6); +// StorageDead(_5); +// goto -> bb4; +// } +// bb4: { +// drop(_1) -> bb5; +// } +// bb5: { +// return; +// } +// } +// END rustc.match_dtor.SimplifyCfg-final.after.mir From b2a9b6b90ae2760175d96d087a24902f663634b5 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 24 Jan 2020 11:45:59 +0900 Subject: [PATCH 6/8] Check statements length --- src/librustc_mir/transform/simplify_try.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index e77e1f12c2510..94cd56b49f546 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -153,6 +153,11 @@ fn match_arm<'tcx>( _ => (0, None), }; + if idx + 8 != stmts.len() { + // There should be other 8 statements than the one assigining to drop flag. + return; + } + // _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); let (local_tmp, local_1, vf_1) = match match_get_variant_field(&stmts[idx + 1]) { None => return, From f7640ff856727b29a4eb583f11a440ace227e819 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Mon, 27 Jan 2020 07:35:01 +0900 Subject: [PATCH 7/8] Add/fix comment --- src/librustc_mir/transform/simplify_try.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index 94cd56b49f546..fce05edfb8aa3 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -56,9 +56,7 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity { _ => {} } - if 8 <= bb.statements.len() && bb.statements.len() <= 9 { - match_arm(&mut bb.statements, local_decls, tcx, param_env); - } + match_arm(&mut bb.statements, local_decls, tcx, param_env); } } } @@ -127,7 +125,10 @@ fn match_arm<'tcx>( tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, ) { - if stmts.len() < 8 || stmts.len() > 9 { + // This function will match on `Variant(x) => Variant(x)`. + // 8 is for the case when `x` does not have a destructor. + // 9 is for the case when `x` does have a destructor and there's an assignment to the drop flag. + if !(8..=9).contains(&stmts.len()) { return; } @@ -378,6 +379,8 @@ impl<'tcx> MirPass<'tcx> for SimplifyBranchSame { } } +/// Extracts a common postfix of statements from all predecessors of a basic block, +/// and inserts these statements at the beginning of the block. pub struct SinkCommonCodeFromPredecessors; impl<'tcx> MirPass<'tcx> for SinkCommonCodeFromPredecessors { @@ -392,6 +395,7 @@ impl<'tcx> MirPass<'tcx> for SinkCommonCodeFromPredecessors { }; let basic_blocks = body.basic_blocks(); + // `predecessors()` gives non-deduplicated list of predecessors. preds.sort_unstable(); preds.dedup(); let is_cleanup = basic_blocks[bb].is_cleanup; @@ -407,6 +411,8 @@ impl<'tcx> MirPass<'tcx> for SinkCommonCodeFromPredecessors { let mut matched_stmts = 0; + // This loop goes backwards in all `preds` statements, + // and gives the number of statements that are the same. loop { if let Some(stmt) = basic_blocks[preds[0]].statements.iter().nth_back(matched_stmts) { From 99a5c541011849e1376b94169a93f72c4f1d9fb3 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Mon, 27 Jan 2020 07:51:07 +0900 Subject: [PATCH 8/8] Fix comment --- src/librustc_mir/transform/simplify_try.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/simplify_try.rs b/src/librustc_mir/transform/simplify_try.rs index fce05edfb8aa3..ef05839321262 100644 --- a/src/librustc_mir/transform/simplify_try.rs +++ b/src/librustc_mir/transform/simplify_try.rs @@ -155,7 +155,7 @@ fn match_arm<'tcx>( }; if idx + 8 != stmts.len() { - // There should be other 8 statements than the one assigining to drop flag. + // There should be 8 statements other than the one assigning to the drop flag. return; }