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

Sink unused const assignments #106613

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
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: 1 addition & 1 deletion compiler/rustc_mir_transform/src/const_goto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub struct ConstGoto;

impl<'tcx> MirPass<'tcx> for ConstGoto {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
sess.mir_opt_level() >= 4
sess.mir_opt_level() >= 1
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ mod ssa;
pub mod simplify;
mod simplify_branches;
mod simplify_comparison_integral;
mod sink_const_assignments;
mod sroa;
mod uninhabited_enum_branching;
mod unreachable_prop;
Expand Down Expand Up @@ -574,6 +575,8 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
&early_otherwise_branch::EarlyOtherwiseBranch,
&simplify_comparison_integral::SimplifyComparisonIntegral,
&dead_store_elimination::DeadStoreElimination,
&sink_const_assignments::SinkConstAssignments,
&const_goto::ConstGoto,
&dest_prop::DestinationPropagation,
&o1(simplify_branches::SimplifyConstCondition::new("final")),
&o1(remove_noop_landing_pads::RemoveNoopLandingPads),
Expand Down
222 changes: 222 additions & 0 deletions compiler/rustc_mir_transform/src/sink_const_assignments.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
use crate::MirPass;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::visit::{NonUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;

const SUCCESSOR_LIMIT: usize = 100;

pub struct SinkConstAssignments;

impl<'tcx> MirPass<'tcx> for SinkConstAssignments {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
sess.mir_opt_level() >= 1
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
// The primary benefit of this pass is sinking assignments to drop flags and enabling
// ConstGoto and SimplifyCfg to merge the drop flag check into existing control flow.
// If we permit sinking assignments to any local, we will sometimes sink an assignment into
// but not completely through a goto chain, preventing SimplifyCfg from removing the
// blocks.
let mut optimizable_locals = branched_locals(body);
if optimizable_locals.is_empty() {
return;
}

let borrowed_locals = rustc_mir_dataflow::impls::borrowed_locals(body);
optimizable_locals.subtract(&borrowed_locals);
if optimizable_locals.is_empty() {
return;
}

'outer: for block in 0..body.basic_blocks.len() {
let block = block.into();
let block_data = &body.basic_blocks[block];
let Some(terminator) = &block_data.terminator else { continue; };

let mut successors = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should bail out when there are too many successors. It's probably not worth creating 100 new assign statements for a 100-arm match

for succ in terminator.successors() {
// Successors which are just a Resume are okay
if is_empty_resume(&body.basic_blocks[succ]) {
continue;
}
if body.basic_blocks.predecessors()[succ].len() != 1 {
debug!("Bailing from {block:?} because {succ:?} has multiple predecessors");
continue 'outer;
}
successors.push(succ);
}

if successors.len() > SUCCESSOR_LIMIT {
debug!("Will not sink assignment, its basic block has too many successors");
}

let mut local_uses = None;
for statement_idx in 0..body.basic_blocks[block].statements.len() {
let statement = &body.basic_blocks[block].statements[statement_idx];
if let StatementKind::Assign(box (place, Rvalue::Use(Operand::Constant(_)))) =
&statement.kind
{
let local = place.local;
if !place.projection.is_empty() {
debug!("Nonempty place projection: {statement:?}");
continue;
}
if !optimizable_locals.contains(local) {
continue;
}

let uses = match &local_uses {
Some(uses) => uses,
None => {
let mut visitor = CountUsesVisitor::new();
visitor.visit_basic_block_data(block, &body.basic_blocks[block]);
local_uses = Some(visitor);
local_uses.as_ref().unwrap()
}
};

// If the local dies in this block, don't propagate it
if uses.dead.contains(&local) {
continue;
}
if !uses.is_used_once(local) {
debug!("Local used elsewhere in this block: {statement:?}");
continue;
}
if !tcx.consider_optimizing(|| format!("Sinking const assignment to {local:?}"))
{
debug!("optimization fuel exhausted");
break 'outer;
}
debug!("Sinking const assignment to {local:?}");
let blocks = body.basic_blocks.as_mut_preserves_cfg();
let statement = blocks[block].statements[statement_idx].replace_nop();

for succ in &successors {
let mut successor_uses = SuccessorUsesVisitor::new(local);
successor_uses.visit_basic_block_data(*succ, &blocks[*succ]);

if let Some(used) = successor_uses.first_use() {
if used == blocks[*succ].statements.len() {
blocks[*succ].statements.push(statement.clone());
continue;
}
// If the first use of our local in this block is another const
// assignment to it, do not paste a new assignment right before it
// because that would just create dead code.
if let StatementKind::Assign(box (
place,
Rvalue::Use(Operand::Constant(_)),
)) = &blocks[*succ].statements[used].kind
{
if place.local == local && place.projection.is_empty() {
continue;
}
}
blocks[*succ].statements.insert(used, statement.clone());
} else {
blocks[*succ].statements.push(statement.clone());
}
}
}
}
}
}
}

fn branched_locals(body: &Body<'_>) -> BitSet<Local> {
let mut visitor = BranchedLocals {
branched: BitSet::new_empty(body.local_decls.len()),
gets_const_assign: BitSet::new_empty(body.local_decls.len()),
};
visitor.visit_body(body);
visitor.branched.intersect(&visitor.gets_const_assign);
visitor.branched
}

struct BranchedLocals {
branched: BitSet<Local>,
gets_const_assign: BitSet<Local>,
}

impl Visitor<'_> for BranchedLocals {
fn visit_terminator(&mut self, terminator: &Terminator<'_>, _location: Location) {
let TerminatorKind::SwitchInt { discr, .. } = &terminator.kind else { return; };
if let Some(place) = discr.place() {
self.branched.insert(place.local);
}
}

fn visit_statement(&mut self, statement: &Statement<'_>, _location: Location) {
if let StatementKind::Assign(box (place, Rvalue::Use(Operand::Constant(_)))) =
&statement.kind
{
if place.projection.is_empty() {
self.gets_const_assign.insert(place.local);
}
}
}
}

fn is_empty_resume<'tcx>(block: &BasicBlockData<'tcx>) -> bool {
block.statements.iter().all(|s| matches!(s.kind, StatementKind::Nop))
&& block.terminator.as_ref().map(|t| &t.kind) == Some(&TerminatorKind::Resume)
}

struct CountUsesVisitor {
counts: FxHashMap<Local, usize>,
dead: FxHashSet<Local>,
}

impl CountUsesVisitor {
fn new() -> Self {
Self { counts: FxHashMap::default(), dead: FxHashSet::default() }
}

fn is_used_once(&self, local: Local) -> bool {
self.counts.get(&local) == Some(&1)
}
}

impl Visitor<'_> for CountUsesVisitor {
fn visit_local(&mut self, local: Local, context: PlaceContext, _location: Location) {
match context {
PlaceContext::NonUse(NonUseContext::StorageDead) => {
self.dead.insert(local);
}
PlaceContext::NonUse(_) => {}
PlaceContext::MutatingUse(_) | PlaceContext::NonMutatingUse(_) => {
*self.counts.entry(local).or_default() += 1;
}
};
}
}

struct SuccessorUsesVisitor {
local: Local,
first_use: Option<usize>,
}

impl SuccessorUsesVisitor {
fn new(local: Local) -> Self {
Self { local, first_use: None }
}

fn first_use(&self) -> Option<usize> {
self.first_use
}
}

impl Visitor<'_> for SuccessorUsesVisitor {
fn visit_local(&mut self, local: Local, _context: PlaceContext, location: Location) {
if local == self.local {
match self.first_use {
None => self.first_use = Some(location.statement_index),
Some(first) => self.first_use = Some(first.min(location.statement_index)),
}
}
}
}
38 changes: 7 additions & 31 deletions tests/mir-opt/const_goto.issue_77355_opt.ConstGoto.diff
Original file line number Diff line number Diff line change
Expand Up @@ -4,48 +4,24 @@
fn issue_77355_opt(_1: Foo) -> u64 {
debug num => _1; // in scope 0 at $DIR/const_goto.rs:+0:20: +0:23
let mut _0: u64; // return place in scope 0 at $DIR/const_goto.rs:+0:33: +0:36
- let mut _2: bool; // in scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- let mut _3: isize; // in scope 0 at $DIR/const_goto.rs:+1:22: +1:28
+ let mut _2: isize; // in scope 0 at $DIR/const_goto.rs:+1:22: +1:28
let mut _2: isize; // in scope 0 at $DIR/const_goto.rs:+1:22: +1:28

bb0: {
- StorageLive(_2); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- _3 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:+1:17: +1:20
- switchInt(move _3) -> [1: bb2, 2: bb2, otherwise: bb1]; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ _2 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:+1:17: +1:20
+ switchInt(move _2) -> [1: bb2, 2: bb2, otherwise: bb1]; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
_2 = discriminant(_1); // scope 0 at $DIR/const_goto.rs:+1:17: +1:20
switchInt(move _2) -> [1: bb2, 2: bb2, otherwise: bb1]; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
}

bb1: {
- _2 = const false; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- goto -> bb3; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ _0 = const 42_u64; // scope 0 at $DIR/const_goto.rs:+1:53: +1:55
+ goto -> bb3; // scope 0 at $DIR/const_goto.rs:+1:5: +1:57
_0 = const 42_u64; // scope 0 at $DIR/const_goto.rs:+1:53: +1:55
goto -> bb3; // scope 0 at $DIR/const_goto.rs:+1:5: +1:57
}

bb2: {
- _2 = const true; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- goto -> bb3; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- }
-
- bb3: {
- switchInt(move _2) -> [0: bb5, otherwise: bb4]; // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
- }
-
- bb4: {
_0 = const 23_u64; // scope 0 at $DIR/const_goto.rs:+1:41: +1:43
- goto -> bb6; // scope 0 at $DIR/const_goto.rs:+1:5: +1:57
+ goto -> bb3; // scope 0 at $DIR/const_goto.rs:+1:5: +1:57
goto -> bb3; // scope 0 at $DIR/const_goto.rs:+1:5: +1:57
}

- bb5: {
- _0 = const 42_u64; // scope 0 at $DIR/const_goto.rs:+1:53: +1:55
- goto -> bb6; // scope 0 at $DIR/const_goto.rs:+1:5: +1:57
- }
-
- bb6: {
- StorageDead(_2); // scope 0 at $DIR/const_goto.rs:+1:56: +1:57
+ bb3: {
bb3: {
return; // scope 0 at $DIR/const_goto.rs:+2:2: +2:2
}
}
Expand Down
20 changes: 5 additions & 15 deletions tests/mir-opt/const_goto_const_eval_fail.f.ConstGoto.diff
Original file line number Diff line number Diff line change
Expand Up @@ -14,35 +14,25 @@
}

bb1: {
_1 = const true; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+4:18: +4:22
goto -> bb3; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+4:18: +4:22
}

bb2: {
_1 = const B; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+3:26: +3:27
- goto -> bb3; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+3:26: +3:27
+ switchInt(_1) -> [0: bb4, otherwise: bb3]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+1:5: +6:6
switchInt(_1) -> [0: bb4, otherwise: bb3]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+1:5: +6:6
}

bb3: {
- switchInt(_1) -> [0: bb5, otherwise: bb4]; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+1:5: +6:6
- }
-
- bb4: {
_0 = const 2_u64; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+8:17: +8:18
- goto -> bb6; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+8:17: +8:18
+ goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+8:17: +8:18
goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+8:17: +8:18
}

- bb5: {
+ bb4: {
bb4: {
_0 = const 1_u64; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+7:18: +7:19
- goto -> bb6; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+7:18: +7:19
+ goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+7:18: +7:19
goto -> bb5; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+7:18: +7:19
}

- bb6: {
+ bb5: {
bb5: {
StorageDead(_2); // scope 0 at $DIR/const_goto_const_eval_fail.rs:+10:1: +10:2
StorageDead(_1); // scope 0 at $DIR/const_goto_const_eval_fail.rs:+10:1: +10:2
return; // scope 0 at $DIR/const_goto_const_eval_fail.rs:+10:2: +10:2
Expand Down
Loading