diff --git a/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs b/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs index 76a8ca13b61b9..59aac982c5dfd 100644 --- a/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs +++ b/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs @@ -145,11 +145,11 @@ impl<'cx, 'tcx> Visitor<'tcx> for LoanInvalidationsGenerator<'cx, 'tcx> { self.mutate_place(location, *resume_arg, Deep); } TerminatorKind::Reattach { continuation } => { - // FIXME(jhilton): Should we be invalidating locals in the current basic block as well? I think we should be, + // FIXME(jhilton): Should we be invalidating locals in the current basic block as well? I think we should be, // for consistency with what we did in the type-checker. My concern is that we only want to kill locals for the // *current block*, which I don't think we're currently expressing. One way to think about modeling this // is that we can imagine Detach as pushing a new stack frame for the spawned task so that way we don't - // invalidate all locals, only the locals that are generated in the spawned task. Alternatively, loan invalidation + // invalidate all locals, only the locals that are generated in the spawned task. Alternatively, loan invalidation // can be handled by however blocks handle this situation. let borrow_set = self.borrow_set; let end_spawned_task = diff --git a/compiler/rustc_data_structures/src/work_queue.rs b/compiler/rustc_data_structures/src/work_queue.rs index 9db6b6f20bede..360dba07d4a78 100644 --- a/compiler/rustc_data_structures/src/work_queue.rs +++ b/compiler/rustc_data_structures/src/work_queue.rs @@ -42,3 +42,11 @@ impl WorkQueue { } } } + +impl Extend for WorkQueue { + fn extend>(&mut self, iter: I) { + for element in iter { + self.insert(element); + } + } +} diff --git a/compiler/rustc_index/src/bit_set.rs b/compiler/rustc_index/src/bit_set.rs index 12f8e42c78f94..54bd2bc676b34 100644 --- a/compiler/rustc_index/src/bit_set.rs +++ b/compiler/rustc_index/src/bit_set.rs @@ -677,8 +677,63 @@ impl BitRelations> for ChunkedBitSet { unimplemented!("implement if/when necessary"); } - fn intersect(&mut self, _other: &ChunkedBitSet) -> bool { - unimplemented!("implement if/when necessary"); + fn intersect(&mut self, other: &ChunkedBitSet) -> bool { + assert_eq!(self.domain_size, other.domain_size); + debug_assert_eq!(self.chunks.len(), other.chunks.len()); + + let mut changed = false; + for (mut self_chunk, other_chunk) in self.chunks.iter_mut().zip(other.chunks.iter()) { + match (&mut self_chunk, &other_chunk) { + (_, Ones(_)) | (Zeros(_), _) => {} + (Ones(self_chunk_domain_size), Zeros(other_chunk_domain_size)) + | (Ones(self_chunk_domain_size), Mixed(other_chunk_domain_size, ..)) + | (Mixed(self_chunk_domain_size, ..), Zeros(other_chunk_domain_size, ..)) => { + // `other_chunk` fully overwrites `self_chunk` + debug_assert_eq!(self_chunk_domain_size, other_chunk_domain_size); + *self_chunk = other_chunk.clone(); + changed = true; + } + ( + Mixed( + self_chunk_domain_size, + ref mut self_chunk_count, + ref mut self_chunk_words, + ), + Mixed(other_chunk_domain_size, _other_chunk_count, other_chunk_words), + ) => { + debug_assert_eq!(self_chunk_domain_size, other_chunk_domain_size); + // First check if the operation would change + // `self_chunk.words`. If not, we can avoid allocating some + // words, and this happens often enough that it's a + // performance win. Also, we only need to operate on the + // in-use words, hence the slicing. + let op = |a, b| a & b; + let num_words = num_words(*self_chunk_domain_size as usize); + if bitwise_changes( + &self_chunk_words[0..num_words], + &other_chunk_words[0..num_words], + op, + ) { + let self_chunk_words = Rc::make_mut(self_chunk_words); + let has_changed = bitwise( + &mut self_chunk_words[0..num_words], + &other_chunk_words[0..num_words], + op, + ); + debug_assert!(has_changed); + *self_chunk_count = self_chunk_words[0..num_words] + .iter() + .map(|w| w.count_ones() as ChunkSize) + .sum(); + if *self_chunk_count == *self_chunk_domain_size { + *self_chunk = Ones(*self_chunk_domain_size); + } + changed = true; + } + } + } + } + changed } } diff --git a/compiler/rustc_index/src/bit_set/tests.rs b/compiler/rustc_index/src/bit_set/tests.rs index 351d62feed949..a44191df6759a 100644 --- a/compiler/rustc_index/src/bit_set/tests.rs +++ b/compiler/rustc_index/src/bit_set/tests.rs @@ -437,6 +437,92 @@ fn chunked_bitset_iter() { check_iter(&bit, &vec); } +#[test] +fn chunked_bitset_intersect_empty_empty() { + let vec: Vec = Vec::new(); + let n = 10000; + let mut bit1 = with_elements_chunked(&vec, n); + let mut bit2 = with_elements_chunked(&vec, n); + assert!(!bit1.intersect(&bit2)); + assert_eq!(bit1, bit2); + assert!(!bit2.intersect(&bit1)); + assert_eq!(bit1, bit2); +} + +#[test] +fn chunked_bitset_intersect_empty_filled() { + let n = 10000; + let vec1: Vec = Vec::new(); + let vec2: Vec = (0..n).collect(); + let mut bit1 = with_elements_chunked(&vec1, n); + let mut bit2 = with_elements_chunked(&vec2, n); + let bit1_clone = bit1.clone(); + assert!(!bit1.intersect(&bit2)); + assert_eq!(bit1, bit1_clone); + assert!(bit2.intersect(&bit1)); + assert_eq!(bit2, bit1); +} + +#[test] +fn chunked_bitset_intersect_empty_mixed() { + let n = 10000; + let vec1: Vec = Vec::new(); + let vec2: Vec = vec![0, 1, 2, 2010, 2047, 2099, 6000, 6002, 6004]; + let mut bit1 = with_elements_chunked(&vec1, n); + let mut bit2 = with_elements_chunked(&vec2, n); + let bit1_clone = bit1.clone(); + assert!(!bit1.intersect(&bit2)); + assert_eq!(bit1, bit1_clone); + assert!(bit2.intersect(&bit1)); + assert_eq!(bit2, bit1); +} + +#[test] +fn chunked_bitset_intersect_filled_filled() { + let n = 10000; + let vec1: Vec = (0..n).collect(); + let vec2: Vec = (0..n).collect(); + let mut bit1 = with_elements_chunked(&vec1, n); + let mut bit2 = with_elements_chunked(&vec2, n); + let bit1_clone = bit1.clone(); + assert!(!bit1.intersect(&bit2)); + assert_eq!(bit1, bit1_clone); + assert!(!bit2.intersect(&bit1)); + assert_eq!(bit2, bit1); +} + +#[test] +fn chunked_bitset_intersect_filled_mixed() { + let n = 10000; + let vec1: Vec = (0..n).collect(); + let vec2: Vec = vec![0, 1, 2, 2010, 2047, 2099, 6000, 6002, 6004]; + let vec_expected: Vec = vec![0, 1, 2, 2010, 2047, 2099, 6000, 6002, 6004]; + let mut bit1 = with_elements_chunked(&vec1, n); + let mut bit2 = with_elements_chunked(&vec2, n); + let bit_expected = with_elements_chunked(&vec_expected, n); + let bit1_clone = bit1.clone(); + assert!(bit1.intersect(&bit2)); + assert_eq!(bit1, bit_expected); + assert!(!bit2.intersect(&bit1_clone)); + assert_eq!(bit2, bit_expected); +} + +#[test] +fn chunked_bitset_intersect_mixed_mixed() { + let n = 10000; + let vec1: Vec = vec![0, 1, 2, 2010, 5, 10, 20]; + let vec2: Vec = vec![0, 1, 2, 2010, 2047, 2099, 6000, 6002, 6004]; + let vec_expected: Vec = vec![0, 1, 2, 2010]; + let mut bit1 = with_elements_chunked(&vec1, n); + let mut bit2 = with_elements_chunked(&vec2, n); + let expected = with_elements_chunked(&vec_expected, n); + let bit1_clone = bit1.clone(); + assert!(bit1.intersect(&bit2)); + assert_eq!(bit1, expected); + assert!(bit2.intersect(&bit1_clone)); + assert_eq!(bit2, expected); +} + #[test] fn grow() { let mut set: GrowableBitSet = GrowableBitSet::with_capacity(65); diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 35f5a6bfac5f4..d8b8967fea59a 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -553,6 +553,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } + fn should_inject_fake_read(&self, initializer_id: ExprId) -> bool { + // For spawns, we don't want to emit a FakeRead since the value will only be defined on sync. + // They're nested inside a Scope consistently, so we just unwrap the scope. + match self.thir[initializer_id] { + Expr { kind: ExprKind::Scope { value, .. }, .. } => { + !matches!(self.thir[value], Expr { kind: ExprKind::CilkSpawn { .. }, .. }) + } + _ => true, + } + } + pub(super) fn expr_into_pattern( &mut self, mut block: BasicBlock, @@ -566,9 +577,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard, true); unpack!(block = self.expr_into_dest(place, block, initializer_id)); - // Inject a fake read, see comments on `FakeReadCause::ForLet`. - let source_info = self.source_info(irrefutable_pat.span); - self.cfg.push_fake_read(block, source_info, FakeReadCause::ForLet(None), place); + if self.should_inject_fake_read(initializer_id) { + // Inject a fake read, see comments on `FakeReadCause::ForLet`. + let source_info = self.source_info(irrefutable_pat.span); + self.cfg.push_fake_read(block, source_info, FakeReadCause::ForLet(None), place); + } self.schedule_drop_for_binding(var, irrefutable_pat.span, OutsideGuard); block.unit() @@ -597,10 +610,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard, true); unpack!(block = self.expr_into_dest(place, block, initializer_id)); - // Inject a fake read, see comments on `FakeReadCause::ForLet`. - let pattern_source_info = self.source_info(irrefutable_pat.span); - let cause_let = FakeReadCause::ForLet(None); - self.cfg.push_fake_read(block, pattern_source_info, cause_let, place); + if self.should_inject_fake_read(initializer_id) { + // Inject a fake read, see comments on `FakeReadCause::ForLet`. + let pattern_source_info = self.source_info(irrefutable_pat.span); + let cause_let = FakeReadCause::ForLet(None); + self.cfg.push_fake_read(block, pattern_source_info, cause_let, place); + } let ty_source_info = self.source_info(annotation.span); diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs index 720515f262db8..62442d4a92d87 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -3,14 +3,15 @@ use rustc_index::Idx; use rustc_middle::mir::{self, Body, CallReturnPlaces, Location, TerminatorEdges}; use rustc_middle::ty::{self, TyCtxt}; -use crate::drop_flag_effects_for_function_entry; -use crate::drop_flag_effects_for_location; use crate::elaborate_drops::DropFlagState; use crate::framework::SwitchIntEdgeEffects; +use crate::mark_cilk_tasks::TaskTree; use crate::move_paths::{HasMoveData, InitIndex, InitKind, LookupResult, MoveData, MovePathIndex}; use crate::on_lookup_result_bits; use crate::MoveDataParamEnv; use crate::{drop_flag_effects, on_all_children_bits}; +use crate::{drop_flag_effects_for_function_entry, mark_cilk_tasks}; +use crate::{drop_flag_effects_for_location, JoinSemiLattice}; use crate::{lattice, AnalysisDomain, GenKill, GenKillAnalysis, MaybeReachable}; /// `MaybeInitializedPlaces` tracks all places that might be @@ -53,11 +54,27 @@ pub struct MaybeInitializedPlaces<'a, 'tcx> { body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>, skip_unreachable_unwind: bool, + /// Maps basic blocks to the task they are part of. + task_tree: TaskTree, + /// Maps locations to the state of the dataflow analysis at that location. The locations in this + /// map are the last locations of tasks. + state_at_last_locations: rustc_data_structures::fx::FxHashMap< + Location, + MaybeReachable>, + >, } impl<'a, 'tcx> MaybeInitializedPlaces<'a, 'tcx> { pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self { - MaybeInitializedPlaces { tcx, body, mdpe, skip_unreachable_unwind: false } + // FIXME(jhilton): I don't like that this constructor does non-trivial work. Make the task tree a parameter? + MaybeInitializedPlaces { + tcx, + body, + mdpe, + skip_unreachable_unwind: false, + task_tree: TaskTree::from_body(body), + state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(), + } } pub fn skipping_unreachable_unwind(mut self) -> Self { @@ -130,16 +147,25 @@ pub struct MaybeUninitializedPlaces<'a, 'tcx> { mark_inactive_variants_as_uninit: bool, skip_unreachable_unwind: BitSet, + + /// See [MaybeInitializedPlaces::task_tree]. + task_tree: TaskTree, + /// See [MaybeInitializedPlaces::state_at_last_locations] + state_at_last_locations: + rustc_data_structures::fx::FxHashMap>, } impl<'a, 'tcx> MaybeUninitializedPlaces<'a, 'tcx> { pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self { + // FIXME(jhilton): non-trivial work in constructor :( MaybeUninitializedPlaces { tcx, body, mdpe, mark_inactive_variants_as_uninit: false, skip_unreachable_unwind: BitSet::new_empty(body.basic_blocks.len()), + task_tree: TaskTree::from_body(body), + state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(), } } @@ -205,11 +231,19 @@ impl<'a, 'tcx> HasMoveData<'tcx> for MaybeUninitializedPlaces<'a, 'tcx> { pub struct DefinitelyInitializedPlaces<'a, 'tcx> { body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>, + task_tree: TaskTree, + state_at_last_locations: + rustc_data_structures::fx::FxHashMap>>, } impl<'a, 'tcx> DefinitelyInitializedPlaces<'a, 'tcx> { pub fn new(body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self { - DefinitelyInitializedPlaces { body, mdpe } + DefinitelyInitializedPlaces { + body, + mdpe, + task_tree: TaskTree::from_body(body), + state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(), + } } } @@ -251,11 +285,19 @@ impl<'a, 'tcx> HasMoveData<'tcx> for DefinitelyInitializedPlaces<'a, 'tcx> { pub struct EverInitializedPlaces<'a, 'tcx> { body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>, + task_tree: TaskTree, + state_at_last_locations: + rustc_data_structures::fx::FxHashMap>, } impl<'a, 'tcx> EverInitializedPlaces<'a, 'tcx> { pub fn new(body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self { - EverInitializedPlaces { body, mdpe } + EverInitializedPlaces { + body, + mdpe, + task_tree: TaskTree::from_body(body), + state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(), + } } } @@ -374,6 +416,28 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { drop_flag_effects_for_location(self.body, self.mdpe, location, |path, s| { Self::update_bits(state, path, s) }); + + // This lets us track the state before a reattach, which is necessary when we sync. + if let mir::TerminatorKind::Reattach { continuation: _ } = terminator.kind { + self.state_at_last_locations.insert(location, state.clone()); + } else if let mir::TerminatorKind::Sync { target: _ } = terminator.kind { + // Grab the state at all last locations we could be syncing based on the current basic block. + let task = self.task_tree.expect_task(location); + // We skip the locations that don't exist because a task can have children which aren't synced at this point in the dataflow analysis, since + // they can be successors of this sync. This is because tasks don't end on sync. + self.task_tree + .children_last_locations(task) + .filter_map(|last_location| { + self.state_at_last_locations.get(&last_location).cloned() + }) + .for_each(|state_at_last_location| { + // Bottom is uninitialized and top is initialized, and we want to become more initialized, so we go up. + // This makes sense because as we go 'up' in the lattice, we consider more of the state to be initialized. + // `join` provides least-upper-bound and we want the state to become "more initialized" upon a sync. + state.join(&state_at_last_location); + }); + } + edges } @@ -493,6 +557,21 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { drop_flag_effects_for_location(self.body, self.mdpe, location, |path, s| { Self::update_bits(trans, path, s) }); + if let mir::TerminatorKind::Reattach { continuation: _ } = terminator.kind { + self.state_at_last_locations.insert(location, trans.clone()); + } else if let mir::TerminatorKind::Sync { target: _ } = terminator.kind { + let task = self.task_tree.expect_task(location); + // See the comment in `MaybeInitializedPlaces::terminator_effect` for why we skip locations that have no state. + self.task_tree + .children_last_locations(task) + .filter_map(|location| self.state_at_last_locations.get(&location)) + .for_each(|state| { + use crate::lattice::MeetSemiLattice; + // Bottom is all-initialized and top is all-uninitialized, so we want to use meet to go lower in the lattice. + trans.meet(state); + }); + } + if self.skip_unreachable_unwind.contains(location.block) { let mir::TerminatorKind::Drop { target, unwind, .. } = terminator.kind else { bug!() }; assert!(matches!(unwind, mir::UnwindAction::Cleanup(_))); @@ -590,6 +669,30 @@ impl<'a, 'tcx> AnalysisDomain<'tcx> for DefinitelyInitializedPlaces<'a, 'tcx> { } } +impl<'a, 'tcx> DefinitelyInitializedPlaces<'a, 'tcx> { + /// Returns the dataflow state from joining the last locations of a task. + fn state_exiting_task( + &self, + task: mark_cilk_tasks::Task, + ) -> >::Domain { + // HACK(jhilton): this initialization is the exact same as the one in `bottom_value`. That's pretty terrible + // but I don't think a helper function really makes sense, and we can't use `bottom_value` because it expects + // a Body. + let init = lattice::Dual(BitSet::new_filled(self.move_data().move_paths.len())); + self.task_tree + .last_locations(task) + .filter_map(|location| self.state_at_last_locations.get(&location)) + .fold(init, |mut acc, state| { + // We use join here because we want to merge flow results from multiple paths in the + // conventional way. For DefinitelyInitialized, that happens to be intersection by + // the definition of the analysis domain, so we're narrowing the set of deifnitely-initialized + // variables. + acc.join(state); + acc + }) + } +} + impl<'tcx> GenKillAnalysis<'tcx> for DefinitelyInitializedPlaces<'_, 'tcx> { type Idx = MovePathIndex; @@ -617,6 +720,27 @@ impl<'tcx> GenKillAnalysis<'tcx> for DefinitelyInitializedPlaces<'_, 'tcx> { drop_flag_effects_for_location(self.body, self.mdpe, location, |path, s| { Self::update_bits(trans, path, s) }); + + if let mir::TerminatorKind::Reattach { continuation: _ } = terminator.kind { + self.state_at_last_locations.insert(location, trans.clone()); + } else if let mir::TerminatorKind::Sync { target: _ } = terminator.kind { + let task = self.task_tree.expect_task(location); + // We want to say that a state is definitely initialized if it is definitely initialized in some synced child. + // FIXME(jhilton): this is currently inaccurate because we sync locations that we may not have actually reached + // in the case of conditional spawns. We should only sync locations here that we know have actually detached. + // We could figure out which locations have actually detached through a separate gen-kill analysis that we only + // need for this case, and possibly for borrow-checking. This is a SOUNDNESS problem that we should definitely fix. + self.task_tree.children(task).map(|child| self.state_exiting_task(child)).for_each( + |state| { + // We use meet here because we need the number of initialized variables to increase at a sync, + // and meet is union for `DefinitelyInitialized`. It also makes sense since we're going down in the + // lattice by using meet, which takes us closer to all variables being initialized. + use crate::framework::lattice::MeetSemiLattice; + trans.meet(&state); + }, + ); + } + terminator.edges() } @@ -714,6 +838,19 @@ impl<'tcx> GenKillAnalysis<'tcx> for EverInitializedPlaces<'_, 'tcx> { }) .copied(), ); + if let mir::TerminatorKind::Reattach { continuation: _ } = terminator.kind { + self.state_at_last_locations.insert(location, trans.clone()); + } else if let mir::TerminatorKind::Sync { target: _ } = terminator.kind { + let task = self.task_tree.expect_task(location); + self.task_tree + .children_last_locations(task) + .filter_map(|location| self.state_at_last_locations.get(&location)) + .for_each(|state| { + // This lattice has all-uninitialized as the bottom and the join operator adds + // initialized places, so we use join here. + trans.join(state); + }); + } terminator.edges() } diff --git a/compiler/rustc_mir_dataflow/src/lib.rs b/compiler/rustc_mir_dataflow/src/lib.rs index f18a2354301e4..579b50ae26d6f 100644 --- a/compiler/rustc_mir_dataflow/src/lib.rs +++ b/compiler/rustc_mir_dataflow/src/lib.rs @@ -31,6 +31,7 @@ pub mod elaborate_drops; mod errors; mod framework; pub mod impls; +mod mark_cilk_tasks; pub mod move_paths; pub mod points; pub mod rustc_peek; diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs new file mode 100644 index 0000000000000..778cf852173b1 --- /dev/null +++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs @@ -0,0 +1,259 @@ +use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::work_queue::WorkQueue; +use rustc_index::bit_set::BitSet; +use rustc_index::IndexVec; +use rustc_middle::mir::{self, BasicBlock, Location}; + +// We want a visitor that generates a tree of tasks. A task is a child of another task if it was detached from that task +// as the spawned task, while the continuation is part of the same task as the block it detached from. We can then label +// basic blocks with their task. +rustc_index::newtype_index! { + #[debug_format = "t({})"] + pub struct Task {} +} + +struct TaskData { + /// Represents the task which spawned this task. Is only None if this task is an orphan, i.e. the original task in the function. + pub parent: Option, + /// Represents all child tasks that may be spawned by this task. + pub children: Vec, + /// Represents all locations which this task might end at where control will not be returned to it. + pub last_locations: Vec, +} + +pub struct TaskTree { + tasks: IndexVec, + basic_blocks: FxHashMap, + unwind_subgraph: BitSet, + cleanup_blocks: BitSet, +} + +fn cleanup_blocks(body: &mir::Body<'_>) -> BitSet { + let mut cleanup_blocks = BitSet::new_empty(body.basic_blocks.len()); + for (bb, bb_data) in body.basic_blocks.iter_enumerated() { + if bb_data.is_cleanup { + cleanup_blocks.insert(bb); + } + } + cleanup_blocks +} + +/// Compute the blocks reachable from blocks labeled as cleanup. +/// +/// We expect that all basic blocks b in the returned subgraph U +/// are either cleanup blocks or b.predecessors() is a subset of the nodes of U. +fn unwind_subgraph(body: &mir::Body<'_>) -> BitSet { + let mut queue = WorkQueue::with_none(body.basic_blocks.len()); + queue.extend( + body.basic_blocks + .iter_enumerated() + .filter_map(|(bb, bb_data)| bb_data.is_cleanup.then(|| bb)), + ); + let mut cleanup_blocks = BitSet::new_empty(body.basic_blocks.len()); + + // Do a breadth-first search to find all blocks reachable from blocks labeled as cleanup. + while let Some(block) = queue.pop() { + cleanup_blocks.insert(block); + queue.extend(body.basic_blocks[block].terminator().successors()); + } + + // Check the reachability condition. + let predecessors = body.basic_blocks.predecessors(); + for block in cleanup_blocks.iter().filter(|block| !body.basic_blocks[*block].is_cleanup) { + // We now need to know that all predecessors of this block are in the subgraph. + assert!(predecessors[block].iter().all(|pred| cleanup_blocks.contains(*pred))); + } + cleanup_blocks +} + +impl TaskTree { + /// Label the block as part of the given task, panicking if it's already mapped to a different task. + /// + /// This makes sense whenever the block might have been labeled with a task already, but you + /// should always expect that task to be the same: no basic block should be part of two tasks. + fn label_block(&mut self, block: BasicBlock, task: Task) { + assert!( + !self.unwind_subgraph.contains(block), + "expected not to label block in unwind subgraph: unwind subgraph is {:?}, block is {:?}", + self.unwind_subgraph, + block + ); + match self.basic_blocks.entry(block) { + std::collections::hash_map::Entry::Occupied(other_task) + if *other_task.get() != task => + { + panic!( + "expected the task for this block to be the same as the task given: was {:?}, expected {:?}!", + *other_task.get(), + task + ) + } + // In all other cases, we know that it's safe to do this b/c either the mapping doesn't exist, + // and or_insert will just do the insertion, or the mapping does exist and is the same, + // so we don't care that the insertion won't happen. + e => e.or_insert(task), + }; + } + + /// Check invariants of the TaskTree. + #[allow(rustc::potential_query_instability)] + pub fn validate(&self) { + // We're okay with using .values() here because order doesn't matter and we're intentionally trying to check our invariants, + // so if we do see a failure it's not like the order we see a failure in really matters to the end user since it's an ICE anyways. + self.basic_blocks.values().for_each(|task| { + if let Some(task_data) = self.tasks.get(*task) { + for last_location in &task_data.last_locations { + let last_block = last_location.block; + // We need the block with the corresponding last location to be part of the corresponding task. + let mapped_task = self + .basic_blocks + .get(&last_block) + .expect("expected last location's block to have a corresponding task!"); + assert_eq!(mapped_task, task, "expected last_location's block to have the same task as the task it's a last location for!"); + } + } + }); + + let number_of_orphan_tasks = self.tasks.iter().filter(|task| task.parent.is_none()).count(); + assert_eq!( + number_of_orphan_tasks, 1, + "expected exactly 1 orphan task since there should be 1 initially-unlabeled task but found {}!", + number_of_orphan_tasks + ); + } + + fn task(&self, block: BasicBlock) -> Option { + self.basic_blocks.get(&block).copied() + } + + /// Get the task for the given location, panicking if it doesn't exist. + pub fn expect_task(&self, location: Location) -> Task { + self.task(location.block).expect("expected block to have a task!") + } + + /// Get the last locations of the children of this task, panicking if it doesn't exist. + pub fn children_last_locations(&self, task: Task) -> impl Iterator + '_ { + self.children(task).flat_map(move |child| self.last_locations(child)) + } + + /// Get the children of this task, panicking if it doesn't exist. + pub fn children(&self, task: Task) -> impl Iterator + '_ { + self.tasks[task].children.iter().copied() + } + + /// Get the locations where this task may return control to the task its continuation belongs to, panicking if + /// the task doesn't exist. + pub fn last_locations(&self, task: Task) -> impl Iterator + '_ { + self.tasks[task].last_locations.iter().copied() + } + + /// Modify this task tree by detaching from the current task, spawning a new task, and + /// labeling the continuation block with the current task. + fn detach_at( + &mut self, + current_task: Task, + spawned_task: BasicBlock, + continuation: BasicBlock, + ) { + self.label_block(continuation, current_task); + let new_task = self.tasks.push(TaskData { + parent: Some(current_task), + children: vec![], + last_locations: vec![], + }); + self.tasks[current_task].children.push(new_task); + self.label_block(spawned_task, new_task); + } + + /// Modify this task tree by reattaching to the parent task of `current_task` at the given location. + fn reattach_at(&mut self, location: Location, current_task: Task, continuation: BasicBlock) { + let current_task_data = &mut self.tasks[current_task]; + + // Reattach is the only way for the task to change to some other task in a way that + // won't return control to the current task, so we want to add it as a "last location". + current_task_data.last_locations.push(location); + + let parent = + current_task_data.parent.expect("expected current task to have parent if reattaching!"); + // NOTE(jhilton): As long as reattach_at is called only in a preorder traversal, we should expect that + // the block is labeled with the parent task already since the continuation should have + // been visited before we looked at successors of the spawned task. + self.label_block(continuation, parent); + + debug_assert!( + self.tasks[parent].children.contains(¤t_task), + "the current task should be a child of the task being reattached to!" + ); + } + + /// Modify this task tree by adding all blocks that the terminator can go to, to the current task. + fn add_successors_to_current_task( + &mut self, + current_task: Task, + terminator: &mir::Terminator<'_>, + ) { + terminator.successors().for_each(|target| { + if !self.cleanup_blocks.contains(target) { + self.label_block(target, current_task); + } + }); + } + + /// Create a TaskTree from a MIR body. + pub fn from_body<'a, 'tcx>(body: &'a mir::Body<'tcx>) -> Self { + // We use this instead of a visitor because we want to control the iteration order. + // We need to know that all ancestors of a block are visited before the block itself. + let mut task_tree = Self { + tasks: IndexVec::new(), + basic_blocks: FxHashMap::default(), + unwind_subgraph: unwind_subgraph(body), + cleanup_blocks: cleanup_blocks(body), + }; + for (block, block_data) in mir::traversal::preorder(body) { + if task_tree.unwind_subgraph.contains(block) { + continue; + } + + let current_task = *task_tree.basic_blocks.entry(block).or_insert_with(|| { + assert!( + task_tree.tasks.is_empty(), + "expected the first task to be the only orphan task!" + ); + task_tree.tasks.push(TaskData { + parent: None, + children: vec![], + last_locations: vec![], + }) + }); + + // As per Location's docs, we know that the length of statements is the index of the terminator. + let location = Location { block, statement_index: block_data.statements.len() }; + let terminator = block_data.terminator(); + match terminator.kind { + mir::TerminatorKind::Detach { spawned_task, continuation } => { + task_tree.detach_at(current_task, spawned_task, continuation); + } + mir::TerminatorKind::Reattach { continuation } => { + task_tree.reattach_at(location, current_task, continuation); + } + _ => { + // For all other terminators, we want to mark all targets as children of the current task. + // The correct behavior for panics and unwinding is to avoid marking blocks used during + // unwinding as part of any tasks. We should disallow using spawn and sync in unwinding + // contexts. + task_tree.add_successors_to_current_task(current_task, terminator); + } + } + } + task_tree.validate(); + task_tree + } +} + +// FIXME(jhilton): at some point we should do a dataflow analysis on which tasks can be potentially running at a time given that +// basic blocks have been assigned to tasks. The only complicated part is that a sync says that all tasks which could be synced +// are definitely not running, and then this means that we can have a MightRunLogicallyInParallel and a +// DefinitelyLogicallyRunsInParallel. I think that dataflow is a fairly good fit for this since we know that on either side of a branch, +// a spawned task is "confined" to that side. Then when we merge the branch in the successor to the conditional (however that happens), +// we merge properly depending on the operator we use for merging (intersection or union). Without this change, we too-optimistically +// mark variables as initialized if they're initialized in some conditionally-spawned work, which is an obvious soundness bug. diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index 39d784ba10a82..0851a2d8a9d54 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -892,7 +892,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { | mir::TerminatorKind::SwitchInt { .. } | mir::TerminatorKind::UnwindResume | mir::TerminatorKind::Return - // NOTE(jhilton): we don't have to do any monomorphization collection because none of + // NOTE(jhilton): we don't have to do any monomorphization collection because none of // these terminators work with values, only with blocks. | mir::TerminatorKind::Detach { spawned_task: _, continuation: _ } | mir::TerminatorKind::Reattach { continuation: _ } diff --git a/compiler/rustc_passes/src/liveness.rs b/compiler/rustc_passes/src/liveness.rs index 4070f6f09c0c5..2407a76728f23 100644 --- a/compiler/rustc_passes/src/liveness.rs +++ b/compiler/rustc_passes/src/liveness.rs @@ -417,7 +417,9 @@ impl<'tcx> Visitor<'tcx> for IrMaps<'tcx> { self.add_live_node_for_node(expr.hir_id, ExprNode(expr.span, expr.hir_id)); } - // FIXME(jhilton): we have to do a more sophisticated analysis using spawn and sync here. + // NOTE(jhilton): we don't do anything interesting with spawn and sync for liveness because + // liveness propagates through the AST in a non-dataflowy way, so we don't get errors about + // liveness from the AST pass. // otherwise, live nodes are not required: hir::ExprKind::Index(..) diff --git a/tests/ui/cilk/borrows_dropped_after_sync.rs b/tests/ui/cilk/borrows_dropped_after_sync.rs index e6b2e40f822a8..f98364833973d 100644 --- a/tests/ui/cilk/borrows_dropped_after_sync.rs +++ b/tests/ui/cilk/borrows_dropped_after_sync.rs @@ -5,7 +5,7 @@ // rather than at the sync. There's another test that borrows are live before a sync. fn main() { let mut s = String::from("hello"); - cilk_spawn { + cilk_spawn { println!("{}", &s); }; cilk_sync; diff --git a/tests/ui/cilk/borrows_live_before_sync.rs b/tests/ui/cilk/borrows_live_before_sync.rs index c1e450c2ff24e..e0c89b65d2a55 100644 --- a/tests/ui/cilk/borrows_live_before_sync.rs +++ b/tests/ui/cilk/borrows_live_before_sync.rs @@ -2,12 +2,12 @@ // build-pass // known-bug: unknown -// This should be rejected since s is still referenced by the spawned block +// This should be rejected since s is still referenced by the spawned block // and there's no sync to indicate that the borrow can be dropped. fn main() { let mut s = String::from("hello"); - cilk_spawn { + cilk_spawn { println!("{}", &s); }; s.push_str(" world"); diff --git a/tests/ui/cilk/const_cilk_keywords.rs b/tests/ui/cilk/const_cilk_keywords.rs index c0a2edc79ed48..b9aa1357f6f3c 100644 --- a/tests/ui/cilk/const_cilk_keywords.rs +++ b/tests/ui/cilk/const_cilk_keywords.rs @@ -1,5 +1,5 @@ // Check what happens when using Cilk keywords in a const context. -// known-bug: unknown +// build-pass const fn fib(n: usize) -> usize { if n <= 1 { diff --git a/tests/ui/cilk/const_cilk_keywords.stderr b/tests/ui/cilk/const_cilk_keywords.stderr deleted file mode 100644 index ff346db1bdb87..0000000000000 --- a/tests/ui/cilk/const_cilk_keywords.stderr +++ /dev/null @@ -1,12 +0,0 @@ -error[E0381]: used binding `x` is possibly-uninitialized - --> $DIR/const_cilk_keywords.rs:9:9 - | -LL | let x = cilk_spawn { fib(n - 1) }; - | ^ - | | - | `x` used here but it is possibly-uninitialized - | binding declared here but left uninitialized - -error: aborting due to 1 previous error - -For more information about this error, try `rustc --explain E0381`. diff --git a/tests/ui/cilk/error_when_value_returned_from_spawned_block.rs b/tests/ui/cilk/error_when_value_returned_from_spawned_block.rs new file mode 100644 index 0000000000000..84689055256d2 --- /dev/null +++ b/tests/ui/cilk/error_when_value_returned_from_spawned_block.rs @@ -0,0 +1,10 @@ +// Tests that compilation fails when a value is returned from a spawned block before +// a sync runs. + +fn main() { + let x = { + let y = cilk_spawn { 1 }; + y +//~^ ERROR: used binding `y` is possibly-uninitialized [E0381] + }; +} \ No newline at end of file diff --git a/tests/ui/cilk/error_when_value_returned_from_spawned_block.stderr b/tests/ui/cilk/error_when_value_returned_from_spawned_block.stderr new file mode 100644 index 0000000000000..52e0ff34d3bca --- /dev/null +++ b/tests/ui/cilk/error_when_value_returned_from_spawned_block.stderr @@ -0,0 +1,13 @@ +error[E0381]: used binding `y` is possibly-uninitialized + --> $DIR/error_when_value_returned_from_spawned_block.rs:7:9 + | +LL | let y = cilk_spawn { 1 }; + | - ----- binding initialized here in some conditions + | | + | binding declared here but left uninitialized +LL | y + | ^ `y` used here but it is possibly-uninitialized + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0381`. diff --git a/tests/ui/cilk/fib_block_recurse.rs b/tests/ui/cilk/fib_block_recurse.rs index 2c1de63fdf573..09f057c99ffdc 100644 --- a/tests/ui/cilk/fib_block_recurse.rs +++ b/tests/ui/cilk/fib_block_recurse.rs @@ -1,5 +1,5 @@ // Checks that a simple Cilk program compiles. -// known-bug: unknown +// build-pass fn fib(n: usize) -> usize { if n <= 1 { diff --git a/tests/ui/cilk/fib_block_recurse.stderr b/tests/ui/cilk/fib_block_recurse.stderr deleted file mode 100644 index 3763e27e05609..0000000000000 --- a/tests/ui/cilk/fib_block_recurse.stderr +++ /dev/null @@ -1,12 +0,0 @@ -error[E0381]: used binding `x` is possibly-uninitialized - --> $DIR/fib_block_recurse.rs:8:9 - | -LL | let x = cilk_spawn { fib(n - 1) }; - | ^ - | | - | `x` used here but it is possibly-uninitialized - | binding declared here but left uninitialized - -error: aborting due to 1 previous error - -For more information about this error, try `rustc --explain E0381`. diff --git a/tests/ui/cilk/fib_block_recurse_no_sync.rs b/tests/ui/cilk/fib_block_recurse_no_sync.rs index c88bd394ea23d..74cc1f9f58c13 100644 --- a/tests/ui/cilk/fib_block_recurse_no_sync.rs +++ b/tests/ui/cilk/fib_block_recurse_no_sync.rs @@ -1,14 +1,13 @@ // Checks that a cilk program without a sync reports an uninitialized variable error. -// check-fail fn fib(n: usize) -> usize { if n <= 1 { return n; } let x = cilk_spawn { fib(n - 1) }; -//~^ ERROR used binding `x` is possibly-uninitialized [E0381] let y = fib(n - 2); x + y +//~^ ERROR used binding `x` is possibly-uninitialized [E0381] } fn main() { diff --git a/tests/ui/cilk/fib_block_recurse_no_sync.stderr b/tests/ui/cilk/fib_block_recurse_no_sync.stderr index 0cdd74e14bdbc..86e8d873428b0 100644 --- a/tests/ui/cilk/fib_block_recurse_no_sync.stderr +++ b/tests/ui/cilk/fib_block_recurse_no_sync.stderr @@ -1,11 +1,13 @@ error[E0381]: used binding `x` is possibly-uninitialized - --> $DIR/fib_block_recurse_no_sync.rs:8:9 + --> $DIR/fib_block_recurse_no_sync.rs:9:5 | LL | let x = cilk_spawn { fib(n - 1) }; - | ^ + | - -------------- binding initialized here in some conditions | | - | `x` used here but it is possibly-uninitialized | binding declared here but left uninitialized +LL | let y = fib(n - 2); +LL | x + y + | ^ `x` used here but it is possibly-uninitialized error: aborting due to 1 previous error diff --git a/tests/ui/cilk/fib_block_recurse_type_ascription.rs b/tests/ui/cilk/fib_block_recurse_type_ascription.rs new file mode 100644 index 0000000000000..8b1a7164ae382 --- /dev/null +++ b/tests/ui/cilk/fib_block_recurse_type_ascription.rs @@ -0,0 +1,14 @@ +// Checks that a simple Cilk program compiles, with type ascription. +// build-pass + +fn fib(n: usize) -> usize { + if n <= 1 { + return n; + } + let x: usize = cilk_spawn { fib(n - 1) }; + let y: usize = fib(n - 2); + cilk_sync; + x + y +} + +fn main() {} \ No newline at end of file diff --git a/tests/ui/cilk/matching_on_spawned_expression.rs b/tests/ui/cilk/matching_on_spawned_expression.rs new file mode 100644 index 0000000000000..7a77593735db8 --- /dev/null +++ b/tests/ui/cilk/matching_on_spawned_expression.rs @@ -0,0 +1,10 @@ +// Tests that matching on a spawned expression gives an error. + +fn main() { + let x: Option = cilk_spawn { None }; + match x { +//~^ ERROR used binding `x` is possibly-uninitialized [E0381] + Some(x) => {} + None => {} + } +} diff --git a/tests/ui/cilk/matching_on_spawned_expression.stderr b/tests/ui/cilk/matching_on_spawned_expression.stderr new file mode 100644 index 0000000000000..b0ea267152f90 --- /dev/null +++ b/tests/ui/cilk/matching_on_spawned_expression.stderr @@ -0,0 +1,13 @@ +error[E0381]: used binding `x` is possibly-uninitialized + --> $DIR/matching_on_spawned_expression.rs:5:11 + | +LL | let x: Option = cilk_spawn { None }; + | - -------- binding initialized here in some conditions + | | + | binding declared here but left uninitialized +LL | match x { + | ^ `x` used here but it is possibly-uninitialized + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0381`. diff --git a/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.rs b/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.rs index fadbbb73c48e1..497656d9ab3d4 100644 --- a/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.rs +++ b/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.rs @@ -1,6 +1,7 @@ -// Tests that when a variable is declared in a spawned block, it's not usable outside that spawned block. +// Tests that when a variable is declared in a spawned block, it's not usable outside +// that spawned block. fn main() { let _ = cilk_spawn { let y = 5; y }; println!("y={}", y); //~^ ERROR cannot find value `y` in this scope [E0425] -} \ No newline at end of file +} diff --git a/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.stderr b/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.stderr index 05c0b1bc29665..554001af33cff 100644 --- a/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.stderr +++ b/tests/ui/cilk/proper_scoping_of_variables_in_spawned_block.stderr @@ -1,11 +1,11 @@ error[E0425]: cannot find value `y` in this scope - --> $DIR/proper_scoping_of_variables_in_spawned_block.rs:4:22 + --> $DIR/proper_scoping_of_variables_in_spawned_block.rs:5:22 | LL | println!("y={}", y); | ^ | help: the binding `y` is available in a different scope in the same function - --> $DIR/proper_scoping_of_variables_in_spawned_block.rs:3:30 + --> $DIR/proper_scoping_of_variables_in_spawned_block.rs:4:30 | LL | let _ = cilk_spawn { let y = 5; y }; | ^ diff --git a/tests/ui/cilk/require_sync.rs b/tests/ui/cilk/require_sync.rs index cabf841bda45c..5ffb0c949fce0 100644 --- a/tests/ui/cilk/require_sync.rs +++ b/tests/ui/cilk/require_sync.rs @@ -6,6 +6,6 @@ use std::rc::Rc; fn main() { let x = Rc::new(RefCell::new(1_usize)); - // Rc is not Sync so we can't use it here. + // Rc is not Sync so we can't use it here. cilk_spawn { let x = Rc::clone(&x); x.replace_with(|n| *n + 1) }; }