Skip to content

Commit

Permalink
Make it possible for ResultsCursor to borrow a Results.
Browse files Browse the repository at this point in the history
`ResultsCursor` currently owns its `Results`. But sometimes the
`Results` is needed again afterwards. So there is
`ResultsCursor::into_results` for extracting the `Results`, which leads
to some awkwardness.

This commit adds `ResultsHandle`, a `Cow`-like type that can either
borrow or own a a `Results`. `ResultsCursor` now uses it. This is good
because some `ResultsCursor`s really want to own their `Results`, while
others just want to borrow it.

We end with with a few more lines of code, but get some nice cleanups.
- `ResultsCursor::into_results` is removed.
- `Formatter::into_results` is removed.
- `write_graphviz_results` now just borrows a `Results`, instead of the
  awkward "take ownership of a `Results` and then return it unchanged"
  pattern.
- `MaybeRequiresStorage` can borrow the `MaybeBorrowedLocals` results,
  avoiding two `clone` calls: one in `check_for_move` and one in
  `locals_live_across_suspend_points`.
- `Results` no longer needs to derive `Clone`.

This reinstates the cursor flexibility that was lost in #118230 -- which
removed the old `ResultsRefCursor` and `ResultsCloneCursor` types -- but
in a *much* simpler way. Hooray!
  • Loading branch information
nnethercote committed Oct 23, 2024
1 parent cbe3c1e commit 9e735ca
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 37 deletions.
35 changes: 28 additions & 7 deletions compiler/rustc_mir_dataflow/src/framework/cursor.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,39 @@
//! Random access inspection of the results of a dataflow analysis.

use std::cmp::Ordering;
use std::ops::Deref;

#[cfg(debug_assertions)]
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::{self, BasicBlock, Location};

use super::{Analysis, Direction, Effect, EffectIndex, Results};

/// Some `ResultsCursor`s want to own a `Results`, and some want to borrow a `Results`. This type
/// allows either. We could use `Cow` but that would require `Results` and `A` to impl `Clone`. So
/// this is a greatly cut-down alternative to `Cow`.
pub enum ResultsHandle<'a, 'tcx, A>
where
A: Analysis<'tcx>,
{
Borrowed(&'a Results<'tcx, A>),
Owned(Results<'tcx, A>),
}

impl<'tcx, A> Deref for ResultsHandle<'_, 'tcx, A>
where
A: Analysis<'tcx>,
{
type Target = Results<'tcx, A>;

fn deref(&self) -> &Results<'tcx, A> {
match self {
ResultsHandle::Borrowed(borrowed) => borrowed,
ResultsHandle::Owned(owned) => owned,
}
}
}

/// Allows random access inspection of the results of a dataflow analysis.
///
/// This cursor only has linear performance within a basic block when its statements are visited in
Expand All @@ -19,7 +45,7 @@ where
A: Analysis<'tcx>,
{
body: &'mir mir::Body<'tcx>,
results: Results<'tcx, A>,
results: ResultsHandle<'mir, 'tcx, A>,
state: A::Domain,

pos: CursorPosition,
Expand Down Expand Up @@ -47,13 +73,8 @@ where
self.body
}

/// Unwraps this cursor, returning the underlying `Results`.
pub fn into_results(self) -> Results<'tcx, A> {
self.results
}

/// Returns a new cursor that can inspect `results`.
pub fn new(body: &'mir mir::Body<'tcx>, results: Results<'tcx, A>) -> Self {
pub fn new(body: &'mir mir::Body<'tcx>, results: ResultsHandle<'mir, 'tcx, A>) -> Self {
let bottom_value = results.analysis.bottom_value(body);
ResultsCursor {
body,
Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_mir_dataflow/src/framework/graphviz.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,16 @@ where
{
pub(crate) fn new(
body: &'mir Body<'tcx>,
results: Results<'tcx, A>,
results: &'mir Results<'tcx, A>,
style: OutputStyle,
) -> Self {
let reachable = mir::traversal::reachable_as_bitset(body);
Formatter { cursor: results.into_results_cursor(body).into(), style, reachable }
Formatter { cursor: results.as_results_cursor(body).into(), style, reachable }
}

fn body(&self) -> &'mir Body<'tcx> {
self.cursor.borrow().body()
}

pub(crate) fn into_results(self) -> Results<'tcx, A> {
self.cursor.into_inner().into_results()
}
}

/// A pair of a basic block and an index into that basic blocks `successors`.
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_mir_dataflow/src/framework/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,14 +306,13 @@ pub trait Analysis<'tcx> {
let results = Results { analysis: self, entry_sets };

if tcx.sess.opts.unstable_opts.dump_mir_dataflow {
let (res, results) = write_graphviz_results(tcx, body, results, pass_name);
let res = write_graphviz_results(tcx, body, &results, pass_name);
if let Err(e) = res {
error!("Failed to write graphviz dataflow results: {}", e);
}
results
} else {
results
}

results
}
}

Expand Down
30 changes: 20 additions & 10 deletions compiler/rustc_mir_dataflow/src/framework/results.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ use super::{Analysis, ResultsCursor, ResultsVisitor, graphviz, visit_results};
use crate::errors::{
DuplicateValuesFor, PathMustEndInFilename, RequiresAnArgument, UnknownFormatter,
};
use crate::framework::cursor::ResultsHandle;

type EntrySets<'tcx, A> = IndexVec<BasicBlock, <A as Analysis<'tcx>>::Domain>;

/// A dataflow analysis that has converged to fixpoint.
#[derive(Clone)]
pub struct Results<'tcx, A>
where
A: Analysis<'tcx>,
Expand All @@ -34,12 +34,22 @@ impl<'tcx, A> Results<'tcx, A>
where
A: Analysis<'tcx>,
{
/// Creates a `ResultsCursor` that can inspect these `Results`.
/// Creates a `ResultsCursor` that can inspect these `Results`. Only borrows the `Results`,
/// which is appropriate when the `Results` is used outside the cursor.
pub fn as_results_cursor<'mir>(
&'mir self,
body: &'mir mir::Body<'tcx>,
) -> ResultsCursor<'mir, 'tcx, A> {
ResultsCursor::new(body, ResultsHandle::Borrowed(self))
}

/// Creates a `ResultsCursor` that can inspect these `Results`. Takes ownership of the
/// `Results`, which is good when the `Results` is only used by the cursor.
pub fn into_results_cursor<'mir>(
self,
body: &'mir mir::Body<'tcx>,
) -> ResultsCursor<'mir, 'tcx, A> {
ResultsCursor::new(body, self)
ResultsCursor::new(body, ResultsHandle::Owned(self))
}

/// Gets the dataflow state for the given block.
Expand Down Expand Up @@ -74,9 +84,9 @@ where
pub(super) fn write_graphviz_results<'tcx, A>(
tcx: TyCtxt<'tcx>,
body: &mir::Body<'tcx>,
results: Results<'tcx, A>,
results: &Results<'tcx, A>,
pass_name: Option<&'static str>,
) -> (std::io::Result<()>, Results<'tcx, A>)
) -> std::io::Result<()>
where
A: Analysis<'tcx>,
A::Domain: DebugWithContext<A>,
Expand All @@ -87,7 +97,7 @@ where
let def_id = body.source.def_id();
let Ok(attrs) = RustcMirAttrs::parse(tcx, def_id) else {
// Invalid `rustc_mir` attrs are reported in `RustcMirAttrs::parse`
return (Ok(()), results);
return Ok(());
};

let file = try {
Expand All @@ -104,12 +114,12 @@ where
create_dump_file(tcx, "dot", false, A::NAME, &pass_name.unwrap_or("-----"), body)?
}

_ => return (Ok(()), results),
_ => return Ok(()),
}
};
let mut file = match file {
Ok(f) => f,
Err(e) => return (Err(e), results),
Err(e) => return Err(e),
};

let style = match attrs.formatter {
Expand All @@ -119,7 +129,7 @@ where

let mut buf = Vec::new();

let graphviz = graphviz::Formatter::new(body, results, style);
let graphviz = graphviz::Formatter::new(body, &results, style);
let mut render_opts =
vec![dot::RenderOption::Fontname(tcx.sess.opts.unstable_opts.graphviz_font.clone())];
if tcx.sess.opts.unstable_opts.graphviz_dark_mode {
Expand All @@ -132,7 +142,7 @@ where
file.write_all(&buf)?;
};

(lhs, graphviz.into_results())
lhs
}

#[derive(Default)]
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,13 @@ impl<'a, 'tcx> Analysis<'tcx> for MaybeStorageDead<'a> {
/// given location; i.e. whether its storage can go away without being observed.
pub struct MaybeRequiresStorage<'mir, 'tcx> {
body: &'mir Body<'tcx>,
borrowed_locals: Results<'tcx, MaybeBorrowedLocals>,
borrowed_locals: &'mir Results<'tcx, MaybeBorrowedLocals>,
}

impl<'mir, 'tcx> MaybeRequiresStorage<'mir, 'tcx> {
pub fn new(
body: &'mir Body<'tcx>,
borrowed_locals: Results<'tcx, MaybeBorrowedLocals>,
borrowed_locals: &'mir Results<'tcx, MaybeBorrowedLocals>,
) -> Self {
MaybeRequiresStorage { body, borrowed_locals }
}
Expand Down Expand Up @@ -278,7 +278,7 @@ impl<'tcx> Analysis<'tcx> for MaybeRequiresStorage<'_, 'tcx> {
impl<'tcx> MaybeRequiresStorage<'_, 'tcx> {
/// Kill locals that are fully moved and have not been borrowed.
fn check_for_move(&self, trans: &mut <Self as Analysis<'tcx>>::Domain, loc: Location) {
let borrowed_locals = self.borrowed_locals.clone().into_results_cursor(self.body);
let borrowed_locals = self.borrowed_locals.as_results_cursor(self.body);
let mut visitor = MoveVisitor { trans, borrowed_locals };
visitor.visit_location(self.body, loc);
}
Expand Down
13 changes: 6 additions & 7 deletions compiler/rustc_mir_transform/src/coroutine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -673,13 +673,12 @@ fn locals_live_across_suspend_points<'tcx>(
// borrowed (even if they are still active).
let borrowed_locals_results =
MaybeBorrowedLocals.iterate_to_fixpoint(tcx, body, Some("coroutine"));
let mut borrowed_locals_cursor = borrowed_locals_results.clone().into_results_cursor(body);
let mut borrowed_locals_cursor = borrowed_locals_results.as_results_cursor(body);

// Calculate the MIR locals that we actually need to keep storage around
// for.
let mut requires_storage_cursor = MaybeRequiresStorage::new(body, borrowed_locals_results)
.iterate_to_fixpoint(tcx, body, None)
.into_results_cursor(body);
// Calculate the MIR locals for which we need to keep storage around.
let requires_storage_results = MaybeRequiresStorage::new(body, &borrowed_locals_results)
.iterate_to_fixpoint(tcx, body, None);
let mut requires_storage_cursor = requires_storage_results.as_results_cursor(body);

// Calculate the liveness of MIR locals ignoring borrows.
let mut liveness =
Expand Down Expand Up @@ -752,7 +751,7 @@ fn locals_live_across_suspend_points<'tcx>(
body,
&saved_locals,
always_live_locals.clone(),
requires_storage_cursor.into_results(),
requires_storage_results,
);

LivenessInfo {
Expand Down

0 comments on commit 9e735ca

Please sign in to comment.