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

Also consider call and yield as MIR SSA. #113915

Merged
merged 1 commit into from
Oct 10, 2023
Merged
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
37 changes: 22 additions & 15 deletions compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ use rustc_middle::mir::*;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_target::abi::{VariantIdx, FIRST_VARIANT};

use crate::ssa::SsaLocals;
use crate::ssa::{AssignedValue, SsaLocals};
use crate::MirPass;

pub struct GVN;
Expand All @@ -87,21 +87,28 @@ fn propagate_ssa<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let dominators = body.basic_blocks.dominators().clone();

let mut state = VnState::new(tcx, param_env, &ssa, &dominators, &body.local_decls);
for arg in body.args_iter() {
if ssa.is_ssa(arg) {
let value = state.new_opaque().unwrap();
state.assign(arg, value);
}
}

ssa.for_each_assignment_mut(&mut body.basic_blocks, |local, rvalue, location| {
let value = state.simplify_rvalue(rvalue, location).or_else(|| state.new_opaque()).unwrap();
// FIXME(#112651) `rvalue` may have a subtype to `local`. We can only mark `local` as
// reusable if we have an exact type match.
if state.local_decls[local].ty == rvalue.ty(state.local_decls, tcx) {
ssa.for_each_assignment_mut(
body.basic_blocks.as_mut_preserves_cfg(),
|local, value, location| {
let value = match value {
// We do not know anything of this assigned value.
AssignedValue::Arg | AssignedValue::Terminator(_) => None,
// Try to get some insight.
AssignedValue::Rvalue(rvalue) => {
let value = state.simplify_rvalue(rvalue, location);
// FIXME(#112651) `rvalue` may have a subtype to `local`. We can only mark `local` as
// reusable if we have an exact type match.
if state.local_decls[local].ty != rvalue.ty(state.local_decls, tcx) {
return;
}
value
}
};
// `next_opaque` is `Some`, so `new_opaque` must return `Some`.
let value = value.or_else(|| state.new_opaque()).unwrap();
state.assign(local, value);
}
});
},
);

// Stop creating opaques during replacement as it is useless.
state.next_opaque = None;
Expand Down
77 changes: 49 additions & 28 deletions compiler/rustc_mir_transform/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
//! As a consequence of rule 2, we consider that borrowed locals are not SSA, even if they are
//! `Freeze`, as we do not track that the assignment dominates all uses of the borrow.

use either::Either;
use rustc_data_structures::graph::dominators::Dominators;
use rustc_index::bit_set::BitSet;
use rustc_index::{IndexSlice, IndexVec};
Expand All @@ -27,6 +26,12 @@ pub struct SsaLocals {
direct_uses: IndexVec<Local, u32>,
}

pub enum AssignedValue<'a, 'tcx> {
Arg,
Rvalue(&'a mut Rvalue<'tcx>),
Terminator(&'a mut TerminatorKind<'tcx>),
}

impl SsaLocals {
pub fn new<'tcx>(body: &Body<'tcx>) -> SsaLocals {
let assignment_order = Vec::with_capacity(body.local_decls.len());
Expand All @@ -39,6 +44,7 @@ impl SsaLocals {

for local in body.args_iter() {
visitor.assignments[local] = Set1::One(DefLocation::Argument);
visitor.assignment_order.push(local);
}

// For SSA assignments, a RPO visit will see the assignment before it sees any use.
Expand Down Expand Up @@ -105,8 +111,8 @@ impl SsaLocals {
) -> impl Iterator<Item = (Local, &'a Rvalue<'tcx>, Location)> + 'a {
self.assignment_order.iter().filter_map(|&local| {
if let Set1::One(DefLocation::Body(loc)) = self.assignments[local] {
let stmt = body.stmt_at(loc).left()?;
// `loc` must point to a direct assignment to `local`.
let Either::Left(stmt) = body.stmt_at(loc) else { bug!() };
let Some((target, rvalue)) = stmt.kind.as_assign() else { bug!() };
assert_eq!(target.as_local(), Some(local));
Some((local, rvalue, loc))
Expand All @@ -118,18 +124,33 @@ impl SsaLocals {

pub fn for_each_assignment_mut<'tcx>(
&self,
basic_blocks: &mut BasicBlocks<'tcx>,
mut f: impl FnMut(Local, &mut Rvalue<'tcx>, Location),
basic_blocks: &mut IndexSlice<BasicBlock, BasicBlockData<'tcx>>,
mut f: impl FnMut(Local, AssignedValue<'_, 'tcx>, Location),
) {
for &local in &self.assignment_order {
if let Set1::One(DefLocation::Body(loc)) = self.assignments[local] {
// `loc` must point to a direct assignment to `local`.
let bbs = basic_blocks.as_mut_preserves_cfg();
let bb = &mut bbs[loc.block];
let stmt = &mut bb.statements[loc.statement_index];
let StatementKind::Assign(box (target, ref mut rvalue)) = stmt.kind else { bug!() };
assert_eq!(target.as_local(), Some(local));
f(local, rvalue, loc)
match self.assignments[local] {
Set1::One(DefLocation::Argument) => f(
local,
AssignedValue::Arg,
Location { block: START_BLOCK, statement_index: 0 },
),
Set1::One(DefLocation::Body(loc)) => {
let bb = &mut basic_blocks[loc.block];
let value = if loc.statement_index < bb.statements.len() {
// `loc` must point to a direct assignment to `local`.
let stmt = &mut bb.statements[loc.statement_index];
let StatementKind::Assign(box (target, ref mut rvalue)) = stmt.kind else {
bug!()
};
assert_eq!(target.as_local(), Some(local));
AssignedValue::Rvalue(rvalue)
} else {
let term = bb.terminator_mut();
AssignedValue::Terminator(&mut term.kind)
};
f(local, value, loc)
}
_ => {}
}
}
}
Expand Down Expand Up @@ -228,34 +249,34 @@ impl<'tcx> Visitor<'tcx> for SsaVisitor<'_> {
}

fn visit_place(&mut self, place: &Place<'tcx>, ctxt: PlaceContext, loc: Location) {
if place.projection.first() == Some(&PlaceElem::Deref) {
// Do not do anything for storage statements and debuginfo.
let location = match ctxt {
PlaceContext::MutatingUse(
MutatingUseContext::Store | MutatingUseContext::Call | MutatingUseContext::Yield,
) => Some(DefLocation::Body(loc)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmiasko Following the discussion on #116454 (comment), I reuse DefLocation::Body for the terminators. Do you agree with this conclusion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried fixing the handling of call terminators in DefLocation::dominates and in retrospect your earlier approach with an additional variant turned out much simpler (reusing DefLocation::Body requires DefLocation::dominates to access basic blocks and that gets quite inconvenient). Thus, I would propose something along the lines of abd4ad1.

I am not sure if you are still planning to address this as part of this pull request or not. Either way seems fine to me, including leaving DefLocation::Body as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So let's go small steps. This PR as-is, then abd4ad1 in a second PR.

_ => None,
};
if let Some(location) = location
&& let Some(local) = place.as_local()
{
self.assignments[local].insert(location);
if let Set1::One(_) = self.assignments[local] {
// Only record if SSA-like, to avoid growing the vector needlessly.
self.assignment_order.push(local);
}
} else if place.projection.first() == Some(&PlaceElem::Deref) {
// Do not do anything for debuginfo.
if ctxt.is_use() {
// Only change the context if it is a real use, not a "use" in debuginfo.
let new_ctxt = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy);

self.visit_projection(place.as_ref(), new_ctxt, loc);
self.check_dominates(place.local, loc);
}
return;
} else {
self.visit_projection(place.as_ref(), ctxt, loc);
self.visit_local(place.local, ctxt, loc);
}
}

fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, loc: Location) {
if let Some(local) = place.as_local() {
self.assignments[local].insert(DefLocation::Body(loc));
if let Set1::One(_) = self.assignments[local] {
// Only record if SSA-like, to avoid growing the vector needlessly.
self.assignment_order.push(local);
}
} else {
self.visit_place(place, PlaceContext::MutatingUse(MutatingUseContext::Store), loc);
}
self.visit_rvalue(rvalue, loc);
}
}

#[instrument(level = "trace", skip(ssa, body))]
Expand Down
21 changes: 21 additions & 0 deletions tests/mir-opt/copy-prop/calls.multiple_edges.CopyProp.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
- // MIR for `multiple_edges` before CopyProp
+ // MIR for `multiple_edges` after CopyProp

fn multiple_edges(_1: bool) -> u8 {
let mut _0: u8;
let mut _2: u8;

bb0: {
switchInt(_1) -> [1: bb1, otherwise: bb2];
}

bb1: {
_2 = dummy(const 13_u8) -> [return: bb2, unwind continue];
}

bb2: {
_0 = _2;
return;
}
}

24 changes: 24 additions & 0 deletions tests/mir-opt/copy-prop/calls.nrvo.CopyProp.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
- // MIR for `nrvo` before CopyProp
+ // MIR for `nrvo` after CopyProp

fn nrvo() -> u8 {
let mut _0: u8;
let _1: u8;
scope 1 {
- debug y => _1;
+ debug y => _0;
}

bb0: {
- StorageLive(_1);
- _1 = dummy(const 5_u8) -> [return: bb1, unwind continue];
+ _0 = dummy(const 5_u8) -> [return: bb1, unwind continue];
}

bb1: {
- _0 = _1;
- StorageDead(_1);
return;
}
}

43 changes: 43 additions & 0 deletions tests/mir-opt/copy-prop/calls.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Check that CopyProp does propagate return values of call terminators.
// unit-test: CopyProp
// needs-unwind

#![feature(custom_mir, core_intrinsics)]
use std::intrinsics::mir::*;

#[inline(never)]
fn dummy(x: u8) -> u8 {
x
}

// EMIT_MIR calls.nrvo.CopyProp.diff
fn nrvo() -> u8 {
let y = dummy(5); // this should get NRVO
y
}

// EMIT_MIR calls.multiple_edges.CopyProp.diff
#[custom_mir(dialect = "runtime", phase = "initial")]
fn multiple_edges(t: bool) -> u8 {
mir! {
let x: u8;
{
match t { true => bbt, _ => ret }
}
bbt = {
Call(x = dummy(13), ret)
}
ret = {
// `x` is not assigned on the `bb0 -> ret` edge,
// so should not be marked as SSA for merging with `_0`.
RET = x;
Return()
}
}
}

fn main() {
// Make sure the function actually gets instantiated.
nrvo();
multiple_edges(false);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@
+ scope 1 (inlined call_twice::<!, fn() -> ! {sleep}>) {
+ debug f => _2;
+ let mut _3: &fn() -> ! {sleep};
+ let mut _4: !;
+ let _4: !;
+ let mut _5: &fn() -> ! {sleep};
+ let mut _6: !;
+ scope 2 {
+ debug a => _4;
+ let _6: !;
+ scope 3 {
+ debug b => _6;
+ }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
+ let mut _3: &fn() -> ! {sleep};
+ let _4: !;
+ let mut _5: &fn() -> ! {sleep};
+ let mut _6: !;
+ let mut _7: !;
+ scope 2 {
+ debug a => _4;
+ let _6: !;
+ scope 3 {
+ debug b => _6;
+ }
Expand Down
Loading