From 62cdbea8c92ab525a1d546ce010485d10a1fb7b9 Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Wed, 27 Jul 2016 17:46:54 -0700 Subject: [PATCH 1/6] deaggregate structs to enable further optimization --- src/librustc_driver/driver.rs | 2 + src/librustc_mir/transform/deaggregator.rs | 111 +++++++++++++++++++++ src/librustc_mir/transform/mod.rs | 1 + 3 files changed, 114 insertions(+) create mode 100644 src/librustc_mir/transform/deaggregator.rs diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index 9a94cc16bfe8c..657fc6c2c5b15 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -994,6 +994,8 @@ pub fn phase_4_translate_to_llvm<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, passes.push_pass(box mir::transform::no_landing_pads::NoLandingPads); passes.push_pass(box mir::transform::simplify_cfg::SimplifyCfg::new("elaborate-drops")); + passes.push_pass(box mir::transform::deaggregator::Deaggregator); + passes.push_pass(box mir::transform::add_call_guards::AddCallGuards); passes.push_pass(box mir::transform::dump_mir::Marker("PreTrans")); diff --git a/src/librustc_mir/transform/deaggregator.rs b/src/librustc_mir/transform/deaggregator.rs new file mode 100644 index 0000000000000..b1c8a0994038e --- /dev/null +++ b/src/librustc_mir/transform/deaggregator.rs @@ -0,0 +1,111 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use rustc::ty::TyCtxt; +use rustc::mir::repr::*; +use rustc::mir::transform::{MirPass, MirSource, Pass}; +use rustc_data_structures::indexed_vec::Idx; +use rustc::ty::VariantKind; + +pub struct Deaggregator; + +impl Pass for Deaggregator {} + +impl<'tcx> MirPass<'tcx> for Deaggregator { + fn run_pass<'a>(&mut self, tcx: TyCtxt<'a, 'tcx, 'tcx>, + source: MirSource, mir: &mut Mir<'tcx>) { + let node_id = source.item_id(); + let node_path = tcx.item_path_str(tcx.map.local_def_id(node_id)); + debug!("running on: {:?}", node_path); + // we only run when mir_opt_level > 1 + match tcx.sess.opts.debugging_opts.mir_opt_level { + Some(0) | + Some(1) | + None => { return; }, + _ => {} + }; + if let MirSource::Fn(_) = source {} else { return; } + + let mut curr: usize = 0; + for bb in mir.basic_blocks_mut() { + while let Some(idx) = get_aggregate_statement(curr, &bb.statements) { + // do the replacement + debug!("removing statement {:?}", idx); + let src_info = bb.statements[idx].source_info; + let mut suffix_stmts = bb.statements.split_off(idx); + let orig_stmt = suffix_stmts.remove(0); + let StatementKind::Assign(ref lhs, ref rhs) = orig_stmt.kind; + if let &Rvalue::Aggregate(ref agg_kind, ref operands) = rhs { + if let &AggregateKind::Adt(adt_def, variant, substs) = agg_kind { + let n = bb.statements.len(); + bb.statements.reserve(n + operands.len() + suffix_stmts.len()); + for (i, op) in operands.iter().enumerate() { + let ref variant_def = adt_def.variants[variant]; + let ty = variant_def.fields[variant].ty(tcx, substs); + let rhs = Rvalue::Use(op.clone()); + + // since we don't handle enums, we don't need a cast + let lhs_cast = lhs.clone(); + + // if we handled enums: + // let lhs_cast = if adt_def.variants.len() > 1 { + // Lvalue::Projection(Box::new(LvalueProjection { + // base: ai.lhs.clone(), + // elem: ProjectionElem::Downcast(ai.adt_def, ai.variant), + // })) + // } else { + // lhs_cast + // }; + + let lhs_proj = Lvalue::Projection(Box::new(LvalueProjection { + base: lhs_cast, + elem: ProjectionElem::Field(Field::new(i), ty), + })); + let new_statement = Statement { + source_info: src_info, + kind: StatementKind::Assign(lhs_proj, rhs), + }; + debug!("inserting: {:?} @ {:?}", new_statement, idx + i); + bb.statements.push(new_statement); + } + curr = bb.statements.len(); + bb.statements.extend(suffix_stmts); + } + } + } + } + } +} + +fn get_aggregate_statement<'a, 'tcx, 'b>(curr: usize, + statements: &Vec>) + -> Option { + for i in curr..statements.len() { + let ref statement = statements[i]; + let StatementKind::Assign(_, ref rhs) = statement.kind; + if let &Rvalue::Aggregate(ref kind, ref operands) = rhs { + if let &AggregateKind::Adt(adt_def, variant, _) = kind { + if operands.len() > 0 { // don't deaggregate () + if adt_def.variants.len() > 1 { + // only deaggrate structs for now + continue; + } + debug!("getting variant {:?}", variant); + debug!("for adt_def {:?}", adt_def); + let variant_def = &adt_def.variants[variant]; + if variant_def.kind == VariantKind::Struct { + return Some(i); + } + } + } + } + }; + None +} diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 7b707b4adb69a..c3485b8256da1 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -17,3 +17,4 @@ pub mod add_call_guards; pub mod promote_consts; pub mod qualify_consts; pub mod dump_mir; +pub mod deaggregator; From bda46c21fe30377b9587b584c64ffe99da6c14ce Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Tue, 2 Aug 2016 10:46:26 -0700 Subject: [PATCH 2/6] reduce rightward drift, add fixme --- src/librustc_mir/transform/deaggregator.rs | 82 +++++++++++----------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/librustc_mir/transform/deaggregator.rs b/src/librustc_mir/transform/deaggregator.rs index b1c8a0994038e..3d8e013c5a7d8 100644 --- a/src/librustc_mir/transform/deaggregator.rs +++ b/src/librustc_mir/transform/deaggregator.rs @@ -31,55 +31,55 @@ impl<'tcx> MirPass<'tcx> for Deaggregator { None => { return; }, _ => {} }; + + // Do not trigger on constants. Could be revised in future if let MirSource::Fn(_) = source {} else { return; } let mut curr: usize = 0; for bb in mir.basic_blocks_mut() { - while let Some(idx) = get_aggregate_statement(curr, &bb.statements) { - // do the replacement - debug!("removing statement {:?}", idx); - let src_info = bb.statements[idx].source_info; - let mut suffix_stmts = bb.statements.split_off(idx); - let orig_stmt = suffix_stmts.remove(0); - let StatementKind::Assign(ref lhs, ref rhs) = orig_stmt.kind; - if let &Rvalue::Aggregate(ref agg_kind, ref operands) = rhs { - if let &AggregateKind::Adt(adt_def, variant, substs) = agg_kind { - let n = bb.statements.len(); - bb.statements.reserve(n + operands.len() + suffix_stmts.len()); - for (i, op) in operands.iter().enumerate() { - let ref variant_def = adt_def.variants[variant]; - let ty = variant_def.fields[variant].ty(tcx, substs); - let rhs = Rvalue::Use(op.clone()); + let idx = match get_aggregate_statement(curr, &bb.statements) { + Some(idx) => idx, + None => continue, + }; + // do the replacement + debug!("removing statement {:?}", idx); + let src_info = bb.statements[idx].source_info; + let suffix_stmts = bb.statements.split_off(idx+1); + let orig_stmt = bb.statements.pop().unwrap(); + let StatementKind::Assign(ref lhs, ref rhs) = orig_stmt.kind; + let (agg_kind, operands) = match rhs { + &Rvalue::Aggregate(ref agg_kind, ref operands) => (agg_kind, operands), + _ => span_bug!(src_info.span, "expected aggregate, not {:?}", rhs), + }; + let (adt_def, variant, substs) = match agg_kind { + &AggregateKind::Adt(adt_def, variant, substs) => (adt_def, variant, substs), + _ => span_bug!(src_info.span, "expected struct, not {:?}", rhs), + }; + let n = bb.statements.len(); + bb.statements.reserve(n + operands.len() + suffix_stmts.len()); + for (i, op) in operands.iter().enumerate() { + let ref variant_def = adt_def.variants[variant]; + let ty = variant_def.fields[variant].ty(tcx, substs); + let rhs = Rvalue::Use(op.clone()); - // since we don't handle enums, we don't need a cast - let lhs_cast = lhs.clone(); + // since we don't handle enums, we don't need a cast + let lhs_cast = lhs.clone(); - // if we handled enums: - // let lhs_cast = if adt_def.variants.len() > 1 { - // Lvalue::Projection(Box::new(LvalueProjection { - // base: ai.lhs.clone(), - // elem: ProjectionElem::Downcast(ai.adt_def, ai.variant), - // })) - // } else { - // lhs_cast - // }; + // FIXME we cannot deaggregate enums issue: 35186 - let lhs_proj = Lvalue::Projection(Box::new(LvalueProjection { - base: lhs_cast, - elem: ProjectionElem::Field(Field::new(i), ty), - })); - let new_statement = Statement { - source_info: src_info, - kind: StatementKind::Assign(lhs_proj, rhs), - }; - debug!("inserting: {:?} @ {:?}", new_statement, idx + i); - bb.statements.push(new_statement); - } - curr = bb.statements.len(); - bb.statements.extend(suffix_stmts); - } - } + let lhs_proj = Lvalue::Projection(Box::new(LvalueProjection { + base: lhs_cast, + elem: ProjectionElem::Field(Field::new(i), ty), + })); + let new_statement = Statement { + source_info: src_info, + kind: StatementKind::Assign(lhs_proj, rhs), + }; + debug!("inserting: {:?} @ {:?}", new_statement, idx + i); + bb.statements.push(new_statement); } + curr = bb.statements.len(); + bb.statements.extend(suffix_stmts); } } } From e8bfba7dc8a484a3241055eeeaf7ecb733b54fa6 Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Tue, 2 Aug 2016 11:24:55 -0700 Subject: [PATCH 3/6] fix field type, add test --- src/librustc_mir/transform/deaggregator.rs | 2 +- src/test/mir-opt/deaggregator_test.rs | 41 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 src/test/mir-opt/deaggregator_test.rs diff --git a/src/librustc_mir/transform/deaggregator.rs b/src/librustc_mir/transform/deaggregator.rs index 3d8e013c5a7d8..05bdefab40b38 100644 --- a/src/librustc_mir/transform/deaggregator.rs +++ b/src/librustc_mir/transform/deaggregator.rs @@ -59,7 +59,7 @@ impl<'tcx> MirPass<'tcx> for Deaggregator { bb.statements.reserve(n + operands.len() + suffix_stmts.len()); for (i, op) in operands.iter().enumerate() { let ref variant_def = adt_def.variants[variant]; - let ty = variant_def.fields[variant].ty(tcx, substs); + let ty = variant_def.fields[i].ty(tcx, substs); let rhs = Rvalue::Use(op.clone()); // since we don't handle enums, we don't need a cast diff --git a/src/test/mir-opt/deaggregator_test.rs b/src/test/mir-opt/deaggregator_test.rs new file mode 100644 index 0000000000000..e57a9674cf683 --- /dev/null +++ b/src/test/mir-opt/deaggregator_test.rs @@ -0,0 +1,41 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +struct Baz { + x: usize, + y: f32, + z: bool, +} + +fn bar(a: usize) -> Baz { + Baz { x: a, y: 0.0, z: false } +} + +fn main() {} + +// END RUST SOURCE +// START rustc.node13.Deaggregator.before.mir +// bb0: { +// var0 = arg0; // scope 0 at main.rs:8:8: 8:9 +// tmp0 = var0; // scope 1 at main.rs:9:14: 9:15 +// return = Baz { x: tmp0, y: const F32(0), z: const false }; // scope ... +// goto -> bb1; // scope 1 at main.rs:8:1: 10:2 +// } +// END rustc.node13.Deaggregator.before.mir +// START rustc.node13.Deaggregator.after.mir +// bb0: { +// var0 = arg0; // scope 0 at main.rs:8:8: 8:9 +// tmp0 = var0; // scope 1 at main.rs:9:14: 9:15 +// (return.0: usize) = tmp0; // scope 1 at main.rs:9:5: 9:34 +// (return.1: f32) = const F32(0); // scope 1 at main.rs:9:5: 9:34 +// (return.2: bool) = const false; // scope 1 at main.rs:9:5: 9:34 +// goto -> bb1; // scope 1 at main.rs:8:1: 10:2 +// } +// END rustc.node13.Deaggregator.after.mir \ No newline at end of file From d918c990de1e6d0f2a8b3b4a0535de1981029679 Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Tue, 2 Aug 2016 12:30:57 -0700 Subject: [PATCH 4/6] add hashtag to emphasis its a gh issue --- src/librustc_mir/transform/deaggregator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/deaggregator.rs b/src/librustc_mir/transform/deaggregator.rs index 05bdefab40b38..ae91123c69265 100644 --- a/src/librustc_mir/transform/deaggregator.rs +++ b/src/librustc_mir/transform/deaggregator.rs @@ -65,7 +65,7 @@ impl<'tcx> MirPass<'tcx> for Deaggregator { // since we don't handle enums, we don't need a cast let lhs_cast = lhs.clone(); - // FIXME we cannot deaggregate enums issue: 35186 + // FIXME we cannot deaggregate enums issue: #35186 let lhs_proj = Lvalue::Projection(Box::new(LvalueProjection { base: lhs_cast, From d5908a32509ccbbc552179cd75d22c8ea20d4092 Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Tue, 2 Aug 2016 14:47:53 -0700 Subject: [PATCH 5/6] run mir opt test with mir-opt-level=3 so they fire --- src/tools/compiletest/src/runtest.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index f2acfa517ced5..6647a1a0a933d 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -1340,6 +1340,8 @@ actual:\n\ MirOpt => { args.extend(["-Z", "dump-mir=all", + "-Z", + "mir-opt-level=3", "-Z"] .iter() .map(|s| s.to_string())); From 06acf16cdb23dad19b9cf816a55df24d4084823c Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Wed, 3 Aug 2016 11:10:38 -0700 Subject: [PATCH 6/6] reduce rightward drift, add precondition comment --- src/librustc_mir/transform/deaggregator.rs | 35 ++++++++++++---------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/librustc_mir/transform/deaggregator.rs b/src/librustc_mir/transform/deaggregator.rs index ae91123c69265..fccd4a607fdcf 100644 --- a/src/librustc_mir/transform/deaggregator.rs +++ b/src/librustc_mir/transform/deaggregator.rs @@ -34,6 +34,8 @@ impl<'tcx> MirPass<'tcx> for Deaggregator { // Do not trigger on constants. Could be revised in future if let MirSource::Fn(_) = source {} else { return; } + // In fact, we might not want to trigger in other cases. + // Ex: when we could use SROA. See issue #35259 let mut curr: usize = 0; for bb in mir.basic_blocks_mut() { @@ -90,21 +92,24 @@ fn get_aggregate_statement<'a, 'tcx, 'b>(curr: usize, for i in curr..statements.len() { let ref statement = statements[i]; let StatementKind::Assign(_, ref rhs) = statement.kind; - if let &Rvalue::Aggregate(ref kind, ref operands) = rhs { - if let &AggregateKind::Adt(adt_def, variant, _) = kind { - if operands.len() > 0 { // don't deaggregate () - if adt_def.variants.len() > 1 { - // only deaggrate structs for now - continue; - } - debug!("getting variant {:?}", variant); - debug!("for adt_def {:?}", adt_def); - let variant_def = &adt_def.variants[variant]; - if variant_def.kind == VariantKind::Struct { - return Some(i); - } - } - } + let (kind, operands) = match rhs { + &Rvalue::Aggregate(ref kind, ref operands) => (kind, operands), + _ => continue, + }; + let (adt_def, variant) = match kind { + &AggregateKind::Adt(adt_def, variant, _) => (adt_def, variant), + _ => continue, + }; + if operands.len() == 0 || adt_def.variants.len() > 1 { + // don't deaggregate () + // don't deaggregate enums ... for now + continue; + } + debug!("getting variant {:?}", variant); + debug!("for adt_def {:?}", adt_def); + let variant_def = &adt_def.variants[variant]; + if variant_def.kind == VariantKind::Struct { + return Some(i); } }; None