Skip to content
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.

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

[MIR] Deaggregate structs to enable further optimizations #35168

Merged
merged 6 commits into from
Aug 4, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions src/librustc_driver/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"));

Expand Down
111 changes: 111 additions & 0 deletions src/librustc_mir/transform/deaggregator.rs
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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 {
Copy link
Member

@nagisa nagisa Aug 1, 2016

Choose a reason for hiding this comment

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

I’d suggest adding a Pass::should_run(stuff) -> bool. We previously had no use-case for such a method, which is why it does not exist currently, but somebody (…ehem, me…) foresaw such use case and left a useful comment.

Some(0) |
Some(1) |
None => { return; },
_ => {}
};
if let MirSource::Fn(_) = source {} else { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

can you leave a comment as to why we do this if -- iirc, otherwise this triggers in constants. Seems like this may be just a temporary limitation, if we improve our constant handling a la miri.


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);
Copy link
Contributor

@nikomatsakis nikomatsakis Aug 2, 2016

Choose a reason for hiding this comment

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

it'd be more efficient to call split_off(idx+1) and then let orig_stmt = bb.statements.pop().unwrap()

let StatementKind::Assign(ref lhs, ref rhs) = orig_stmt.kind;
if let &Rvalue::Aggregate(ref agg_kind, ref operands) = rhs {
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like this logic is only correct if, indeed, the rhs is an aggregate -- I guess this is assured by get_aggregate_statement above, but can you add an assertion for the "else" case?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, my preferred formatting in this case is like this:

let (agg_kind, operands) = match rhs {
    &Rvalue::Aggregate(ref agg_kind, ref operands) => (agg_kind, operands),
    _ => span_bug!(src_info.span, "expected an aggregate, not {:?}", rhs)
};

This way it is very clear what is happening, and you don't have rightward drift to boot.

if let &AggregateKind::Adt(adt_def, variant, substs) = agg_kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly here

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer to open a FIXME issue about the enum limitation, and describe the problem (that we never set the discriminant). This commented out code can go into the issue description if you like.

// 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<Statement<'tcx>>)
-> Option<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);
}
}
}
}
};
None
}
1 change: 1 addition & 0 deletions src/librustc_mir/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ pub mod add_call_guards;
pub mod promote_consts;
pub mod qualify_consts;
pub mod dump_mir;
pub mod deaggregator;