-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Assorted improvements for rustc_middle::mir::traversal
#116254
Changes from 8 commits
f040210
82e251b
fb0e585
0d8a458
a7f3c4e
e0abb98
0e0dc59
2724243
814fbd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,7 +178,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> { | |
// When we yield `C` and call `traverse_successor`, we push `B` to the stack, but | ||
// since we've already visited `E`, that child isn't added to the stack. The last | ||
// two iterations yield `B` and finally `A` for a final traversal of [E, D, C, B, A] | ||
while let Some(&mut (_, ref mut iter)) = self.visit_stack.last_mut() && let Some(bb) = iter.next_back() { | ||
while let Some(bb) = self.visit_stack.last_mut().and_then(|(_, iter)| iter.next_back()) { | ||
if self.visited.insert(bb) { | ||
if let Some(term) = &self.basic_blocks[bb].terminator { | ||
self.visit_stack.push((bb, term.successors())); | ||
|
@@ -188,16 +188,14 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> { | |
} | ||
} | ||
|
||
impl<'a, 'tcx> Iterator for Postorder<'a, 'tcx> { | ||
type Item = (BasicBlock, &'a BasicBlockData<'tcx>); | ||
impl<'tcx> Iterator for Postorder<'_, 'tcx> { | ||
type Item = BasicBlock; | ||
|
||
fn next(&mut self) -> Option<(BasicBlock, &'a BasicBlockData<'tcx>)> { | ||
let next = self.visit_stack.pop(); | ||
if next.is_some() { | ||
self.traverse_successor(); | ||
} | ||
fn next(&mut self) -> Option<BasicBlock> { | ||
let (bb, _) = self.visit_stack.pop()?; | ||
self.traverse_successor(); | ||
|
||
next.map(|(bb, _)| (bb, &self.basic_blocks[bb])) | ||
Some(bb) | ||
} | ||
|
||
fn size_hint(&self) -> (usize, Option<usize>) { | ||
|
@@ -226,65 +224,6 @@ pub fn postorder<'a, 'tcx>( | |
reverse_postorder(body).rev() | ||
} | ||
|
||
/// Reverse postorder traversal of a graph | ||
/// | ||
/// Reverse postorder is the reverse order of a postorder traversal. | ||
/// This is different to a preorder traversal and represents a natural | ||
/// linearization of control-flow. | ||
/// | ||
/// ```text | ||
/// | ||
/// A | ||
/// / \ | ||
/// / \ | ||
/// B C | ||
/// \ / | ||
/// \ / | ||
/// D | ||
/// ``` | ||
/// | ||
/// A reverse postorder traversal of this graph is either `A B C D` or `A C B D` | ||
/// Note that for a graph containing no loops (i.e., A DAG), this is equivalent to | ||
/// a topological sort. | ||
/// | ||
/// Construction of a `ReversePostorder` traversal requires doing a full | ||
/// postorder traversal of the graph, therefore this traversal should be | ||
/// constructed as few times as possible. Use the `reset` method to be able | ||
/// to re-use the traversal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is very useful to newcomers that do not know the different types of traversal. Can it be preserved somewhere ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about ef3c5bb? |
||
#[derive(Clone)] | ||
pub struct ReversePostorder<'a, 'tcx> { | ||
body: &'a Body<'tcx>, | ||
blocks: Vec<BasicBlock>, | ||
idx: usize, | ||
} | ||
|
||
impl<'a, 'tcx> ReversePostorder<'a, 'tcx> { | ||
pub fn new(body: &'a Body<'tcx>, root: BasicBlock) -> ReversePostorder<'a, 'tcx> { | ||
let blocks: Vec<_> = Postorder::new(&body.basic_blocks, root).map(|(bb, _)| bb).collect(); | ||
let len = blocks.len(); | ||
ReversePostorder { body, blocks, idx: len } | ||
} | ||
} | ||
|
||
impl<'a, 'tcx> Iterator for ReversePostorder<'a, 'tcx> { | ||
type Item = (BasicBlock, &'a BasicBlockData<'tcx>); | ||
|
||
fn next(&mut self) -> Option<(BasicBlock, &'a BasicBlockData<'tcx>)> { | ||
if self.idx == 0 { | ||
return None; | ||
} | ||
self.idx -= 1; | ||
|
||
self.blocks.get(self.idx).map(|&bb| (bb, &self.body[bb])) | ||
} | ||
|
||
fn size_hint(&self) -> (usize, Option<usize>) { | ||
(self.idx, Some(self.idx)) | ||
} | ||
} | ||
|
||
impl<'a, 'tcx> ExactSizeIterator for ReversePostorder<'a, 'tcx> {} | ||
|
||
/// Returns an iterator over all basic blocks reachable from the `START_BLOCK` in no particular | ||
/// order. | ||
/// | ||
|
@@ -298,7 +237,7 @@ pub fn reachable<'a, 'tcx>( | |
/// Returns a `BitSet` containing all basic blocks reachable from the `START_BLOCK`. | ||
pub fn reachable_as_bitset(body: &Body<'_>) -> BitSet<BasicBlock> { | ||
let mut iter = preorder(body); | ||
(&mut iter).for_each(drop); | ||
iter.by_ref().for_each(drop); | ||
iter.visited | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,20 +30,26 @@ pub fn visit_local_usage(locals: &[Local], mir: &Body<'_>, location: Location) - | |
locals.len() | ||
]; | ||
|
||
traversal::ReversePostorder::new(mir, location.block).try_fold(init, |usage, (tbb, tdata)| { | ||
// Give up on loops | ||
if tdata.terminator().successors().any(|s| s == location.block) { | ||
return None; | ||
} | ||
traversal::Postorder::new(&mir.basic_blocks, location.block) | ||
.collect::<Vec<_>>() | ||
.into_iter() | ||
.rev() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the cached traversal be used here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not easily I think, this uses a root block setting, but we only cache this for |
||
.try_fold(init, |usage, tbb| { | ||
let tdata = &mir.basic_blocks[tbb]; | ||
|
||
// Give up on loops | ||
if tdata.terminator().successors().any(|s| s == location.block) { | ||
return None; | ||
} | ||
|
||
let mut v = V { | ||
locals, | ||
location, | ||
results: usage, | ||
}; | ||
v.visit_basic_block_data(tbb, tdata); | ||
Some(v.results) | ||
}) | ||
let mut v = V { | ||
locals, | ||
location, | ||
results: usage, | ||
}; | ||
v.visit_basic_block_data(tbb, tdata); | ||
Some(v.results) | ||
}) | ||
} | ||
|
||
struct V<'a> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use a let chain ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can drop 0e0dc59 if you want :P
My point for using
and_then
was to show thatiter
is only used fornext_back
and is not used in the body of the loop. IMO with let chain it was a bit harder to read