From 744eb2f9372dfe9c4de9faeac27c986fb5c5fb10 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 30 Oct 2024 13:12:03 +1100 Subject: [PATCH 1/3] Rename `BlockFormatter::results` as `BlockFormatter::cursor`. Because it's a `ResultsCursor`, not a `Results`. I find this easier to read and understand. --- .../src/framework/graphviz.rs | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs index 96a70f4fa5b..58bec4b8bb2 100644 --- a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs +++ b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs @@ -96,13 +96,13 @@ where // the value with `None`, move it into the results cursor, move it // back out, and return it to the refcell wrapped in `Some`. let mut fmt = BlockFormatter { - results: results.take().unwrap().into_results_cursor(self.body), + cursor: results.take().unwrap().into_results_cursor(self.body), style: self.style, bg: Background::Light, }; fmt.write_node_label(&mut label, *block).unwrap(); - Some(fmt.results.into_results()) + Some(fmt.cursor.into_results()) }); dot::LabelText::html(String::from_utf8(label).unwrap()) } @@ -155,7 +155,7 @@ struct BlockFormatter<'mir, 'tcx, A> where A: Analysis<'tcx>, { - results: ResultsCursor<'mir, 'tcx, A>, + cursor: ResultsCursor<'mir, 'tcx, A>, bg: Background, style: OutputStyle, } @@ -219,8 +219,8 @@ where // C: State at start of block self.bg = Background::Light; - self.results.seek_to_block_start(block); - let block_start_state = self.results.get().clone(); + self.cursor.seek_to_block_start(block); + let block_start_state = self.cursor.get().clone(); self.write_row_with_full_state(w, "", "(on start)")?; // D + E: Statement and terminator transfer functions @@ -228,12 +228,12 @@ where // F: State at end of block - let terminator = self.results.body()[block].terminator(); + let terminator = self.cursor.body()[block].terminator(); // Write the full dataflow state immediately after the terminator if it differs from the // state at block entry. - self.results.seek_to_block_end(block); - if self.results.get() != &block_start_state || A::Direction::IS_BACKWARD { + self.cursor.seek_to_block_end(block); + if self.cursor.get() != &block_start_state || A::Direction::IS_BACKWARD { let after_terminator_name = match terminator.kind { mir::TerminatorKind::Call { target: Some(_), .. } => "(on unwind)", _ => "(on end)", @@ -250,8 +250,8 @@ where match terminator.kind { mir::TerminatorKind::Call { destination, .. } => { self.write_row(w, "", "(on successful return)", |this, w, fmt| { - let state_on_unwind = this.results.get().clone(); - this.results.apply_custom_effect(|analysis, state| { + let state_on_unwind = this.cursor.get().clone(); + this.cursor.apply_custom_effect(|analysis, state| { analysis.apply_call_return_effect( state, block, @@ -265,9 +265,9 @@ where colspan = this.style.num_state_columns(), fmt = fmt, diff = diff_pretty( - this.results.get(), + this.cursor.get(), &state_on_unwind, - this.results.analysis() + this.cursor.analysis() ), ) })?; @@ -275,8 +275,8 @@ where mir::TerminatorKind::Yield { resume, resume_arg, .. } => { self.write_row(w, "", "(on yield resume)", |this, w, fmt| { - let state_on_coroutine_drop = this.results.get().clone(); - this.results.apply_custom_effect(|analysis, state| { + let state_on_coroutine_drop = this.cursor.get().clone(); + this.cursor.apply_custom_effect(|analysis, state| { analysis.apply_call_return_effect( state, resume, @@ -290,9 +290,9 @@ where colspan = this.style.num_state_columns(), fmt = fmt, diff = diff_pretty( - this.results.get(), + this.cursor.get(), &state_on_coroutine_drop, - this.results.analysis() + this.cursor.analysis() ), ) })?; @@ -302,8 +302,8 @@ where if !targets.is_empty() => { self.write_row(w, "", "(on successful return)", |this, w, fmt| { - let state_on_unwind = this.results.get().clone(); - this.results.apply_custom_effect(|analysis, state| { + let state_on_unwind = this.cursor.get().clone(); + this.cursor.apply_custom_effect(|analysis, state| { analysis.apply_call_return_effect( state, block, @@ -317,9 +317,9 @@ where colspan = this.style.num_state_columns(), fmt = fmt, diff = diff_pretty( - this.results.get(), + this.cursor.get(), &state_on_unwind, - this.results.analysis() + this.cursor.analysis() ), ) })?; @@ -407,9 +407,9 @@ where block: BasicBlock, ) -> io::Result<()> { let diffs = StateDiffCollector::run( - self.results.body(), + self.cursor.body(), block, - self.results.mut_results(), + self.cursor.mut_results(), self.style, ); @@ -420,7 +420,7 @@ where if A::Direction::IS_FORWARD { it.next().unwrap() } else { it.next_back().unwrap() } }; - for (i, statement) in self.results.body()[block].statements.iter().enumerate() { + for (i, statement) in self.cursor.body()[block].statements.iter().enumerate() { let statement_str = format!("{statement:?}"); let index_str = format!("{i}"); @@ -442,7 +442,7 @@ where assert!(diffs_after.is_empty()); assert!(diffs_before.as_ref().map_or(true, ExactSizeIterator::is_empty)); - let terminator = self.results.body()[block].terminator(); + let terminator = self.cursor.body()[block].terminator(); let mut terminator_str = String::new(); terminator.kind.fmt_head(&mut terminator_str).unwrap(); @@ -492,8 +492,8 @@ where mir: &str, ) -> io::Result<()> { self.write_row(w, i, mir, |this, w, fmt| { - let state = this.results.get(); - let analysis = this.results.analysis(); + let state = this.cursor.get(); + let analysis = this.cursor.analysis(); // FIXME: The full state vector can be quite long. It would be nice to split on commas // and use some text wrapping algorithm. From a8ce44f7d9921f4ae1e620016c22f2a0ac6d8753 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 30 Oct 2024 13:12:25 +1100 Subject: [PATCH 2/3] Simplify `graphviz::Formatter`. `Formatter` currently has a `RefCell>` field. This is so the `Results` can be temporarily taken and put into a `ResultsCursor` that is used by `BlockFormatter`, and then put back, which is messy. This commit changes `Formatter` to have a `RefCell` and `BlockFormatter` to have a `&mut ResultsCursor`, which greatly simplifies the code at the `Formatter`/`BlockFormatter` interaction point in `Formatter::node_label`. It also means we construct a `ResultsCursor` once per `Formatter`, instead of once per `node_label` call. The commit also: - documents the reason for the `RefCell`; - adds a `Formatter::body` method, replacing the `Formatter::body` field. --- .../src/framework/graphviz.rs | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs index 58bec4b8bb2..5785964de21 100644 --- a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs +++ b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs @@ -32,8 +32,11 @@ pub(crate) struct Formatter<'mir, 'tcx, A> where A: Analysis<'tcx>, { - body: &'mir Body<'tcx>, - results: RefCell>>, + // The `RefCell` is used because `::node_label` + // takes `&self`, but it needs to modify the cursor. This is also the + // reason for the `Formatter`/`BlockFormatter` split; `BlockFormatter` has + // the operations that involve the mutation, i.e. within the `borrow_mut`. + cursor: RefCell>, style: OutputStyle, reachable: BitSet, } @@ -48,11 +51,15 @@ where style: OutputStyle, ) -> Self { let reachable = mir::traversal::reachable_as_bitset(body); - Formatter { body, results: Some(results).into(), style, reachable } + Formatter { cursor: results.into_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.results.into_inner().unwrap() + self.cursor.into_inner().into_results() } } @@ -81,7 +88,7 @@ where type Edge = CfgEdge; fn graph_id(&self) -> dot::Id<'_> { - let name = graphviz_safe_def_name(self.body.source.def_id()); + let name = graphviz_safe_def_name(self.body().source.def_id()); dot::Id::new(format!("graph_for_def_id_{name}")).unwrap() } @@ -91,19 +98,11 @@ where fn node_label(&self, block: &Self::Node) -> dot::LabelText<'_> { let mut label = Vec::new(); - self.results.replace_with(|results| { - // `Formatter::result` is a `RefCell>` so we can replace - // the value with `None`, move it into the results cursor, move it - // back out, and return it to the refcell wrapped in `Some`. - let mut fmt = BlockFormatter { - cursor: results.take().unwrap().into_results_cursor(self.body), - style: self.style, - bg: Background::Light, - }; + let mut cursor = self.cursor.borrow_mut(); + let mut fmt = + BlockFormatter { cursor: &mut cursor, style: self.style, bg: Background::Light }; + fmt.write_node_label(&mut label, *block).unwrap(); - fmt.write_node_label(&mut label, *block).unwrap(); - Some(fmt.cursor.into_results()) - }); dot::LabelText::html(String::from_utf8(label).unwrap()) } @@ -112,12 +111,12 @@ where } fn edge_label(&self, e: &Self::Edge) -> dot::LabelText<'_> { - let label = &self.body[e.source].terminator().kind.fmt_successor_labels()[e.index]; + let label = &self.body()[e.source].terminator().kind.fmt_successor_labels()[e.index]; dot::LabelText::label(label.clone()) } } -impl<'mir, 'tcx, A> dot::GraphWalk<'mir> for Formatter<'mir, 'tcx, A> +impl<'tcx, A> dot::GraphWalk<'_> for Formatter<'_, 'tcx, A> where A: Analysis<'tcx>, { @@ -125,7 +124,7 @@ where type Edge = CfgEdge; fn nodes(&self) -> dot::Nodes<'_, Self::Node> { - self.body + self.body() .basic_blocks .indices() .filter(|&idx| self.reachable.contains(idx)) @@ -134,10 +133,10 @@ where } fn edges(&self) -> dot::Edges<'_, Self::Edge> { - self.body - .basic_blocks + let body = self.body(); + body.basic_blocks .indices() - .flat_map(|bb| dataflow_successors(self.body, bb)) + .flat_map(|bb| dataflow_successors(body, bb)) .collect::>() .into() } @@ -147,20 +146,20 @@ where } fn target(&self, edge: &Self::Edge) -> Self::Node { - self.body[edge.source].terminator().successors().nth(edge.index).unwrap() + self.body()[edge.source].terminator().successors().nth(edge.index).unwrap() } } -struct BlockFormatter<'mir, 'tcx, A> +struct BlockFormatter<'a, 'mir, 'tcx, A> where A: Analysis<'tcx>, { - cursor: ResultsCursor<'mir, 'tcx, A>, + cursor: &'a mut ResultsCursor<'mir, 'tcx, A>, bg: Background, style: OutputStyle, } -impl<'mir, 'tcx, A> BlockFormatter<'mir, 'tcx, A> +impl<'tcx, A> BlockFormatter<'_, '_, 'tcx, A> where A: Analysis<'tcx>, A::Domain: DebugWithContext, From d921be92a459b41f0ba2d5b97e32397902f37294 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 30 Oct 2024 13:21:32 +1100 Subject: [PATCH 3/3] Return label from `write_node_label`. Instead of appending an empty label. Because it's conceptually simpler. --- .../rustc_mir_dataflow/src/framework/graphviz.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs index 5785964de21..bac75b972f9 100644 --- a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs +++ b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs @@ -97,11 +97,10 @@ where } fn node_label(&self, block: &Self::Node) -> dot::LabelText<'_> { - let mut label = Vec::new(); let mut cursor = self.cursor.borrow_mut(); let mut fmt = BlockFormatter { cursor: &mut cursor, style: self.style, bg: Background::Light }; - fmt.write_node_label(&mut label, *block).unwrap(); + let label = fmt.write_node_label(*block).unwrap(); dot::LabelText::html(String::from_utf8(label).unwrap()) } @@ -172,7 +171,9 @@ where bg } - fn write_node_label(&mut self, w: &mut impl io::Write, block: BasicBlock) -> io::Result<()> { + fn write_node_label(&mut self, block: BasicBlock) -> io::Result> { + use std::io::Write; + // Sample output: // +-+-----------------------------------------------+ // A | bb4 | @@ -199,6 +200,9 @@ where // attributes. Make sure to test the output before trying to remove the redundancy. // Notably, `align` was found to have no effect when applied only to . + let mut v = vec![]; + let w = &mut v; + let table_fmt = concat!( " border=\"1\"", " cellborder=\"1\"", @@ -327,7 +331,9 @@ where _ => {} }; - write!(w, "
") + write!(w, "")?; + + Ok(v) } fn write_block_header_simple(