From 06b4ab1b459882dde4da7464f81b9f8a74a0c40d Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Sun, 31 Mar 2024 18:24:23 -0400
Subject: [PATCH 01/19] Add a module for marking Cilk tasks separately

mark_cilk_tasks builds a task tree and determines all reattach points for a given
task. This is useful when computing various dataflow analyses since sync changes
what variables are initialized (and other state, but that's harder to integrate
and not required to get code to compile).

We still have to use this module to correctly handle sync terminators of basic blocks
when finding what variables will be initialized, it would be nice if we had separate
notions of "will-be-synced" and "may-be-synced" for each corresponding kind of dataflow,
and we still need to integrate this with borrow-checking so that we don't kill loans
too early.
---
 compiler/rustc_mir_dataflow/src/lib.rs        |   1 +
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs | 162 ++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs

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..edb5d5c00d0d5
--- /dev/null
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -0,0 +1,162 @@
+use rustc_data_structures::fx::FxHashMap;
+use rustc_index::IndexVec;
+use rustc_middle::mir::{self, visit::Visitor, BasicBlock};
+
+// 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<Task>,
+    /// Represents all child tasks that may be spawned by this task.
+    pub children: Vec<Task>,
+    /// Represents all locations which this task might end at where control will not be returned to it.
+    pub last_locations: Vec<mir::Location>,
+}
+
+pub struct TaskTree {
+    tasks: IndexVec<Task, TaskData>,
+    basic_blocks: FxHashMap<BasicBlock, Task>,
+}
+
+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) {
+        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!")
+            }
+            // 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
+        );
+    }
+
+    pub fn new() -> Self {
+        Self {
+            tasks: IndexVec::new(),
+            basic_blocks: rustc_data_structures::fx::FxHashMap::default(),
+        }
+    }
+}
+
+impl<'tcx> Visitor<'tcx> for TaskTree {
+    fn visit_terminator(
+        &mut self,
+        terminator: &rustc_middle::mir::Terminator<'tcx>,
+        location: rustc_middle::mir::Location,
+    ) {
+        // If we see a terminator, we want to mark the reachable blocks as being part of
+        // the current task, unless this is a Detach, in which case the spawned task is part
+        // of a new task. On a reattach, the task should be marked as the parent of whatever task
+        // this basic block is part of.
+
+        // This makes sense because we expect it to only happen once. When we finalize the analysis, we'll make sure
+        // that there's exactly one task with no parent (an orphan task).
+        let current_task = *self.basic_blocks.entry(location.block).or_insert_with(|| {
+            self.tasks.push(TaskData { parent: None, children: vec![], last_locations: vec![] })
+        });
+        match terminator.kind {
+            mir::TerminatorKind::Detach { spawned_task, continuation } => {
+                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);
+            }
+            mir::TerminatorKind::Reattach { continuation } => {
+                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 old 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!");
+                self.label_block(continuation, parent);
+
+                debug_assert!(
+                    self.tasks[parent].children.contains(&current_task),
+                    "the current task should be a child of the task being reattached to!"
+                );
+            }
+            _ => {
+                // For all other terminators, we want to mark all targets as children of the current task.
+                // This might have the wrong semantics with panics and unwinding? Hopefully sync insertion
+                // can make that a nonissue.
+                match terminator.edges() {
+                    mir::TerminatorEdges::None => {
+                        // No targets, nothing to do
+                    }
+                    mir::TerminatorEdges::Single(target) => {
+                        self.label_block(target, current_task);
+                    }
+                    mir::TerminatorEdges::Double(target1, target2) => {
+                        self.label_block(target1, current_task);
+                        self.label_block(target2, current_task);
+                    }
+                    mir::TerminatorEdges::AssignOnReturn { return_, cleanup, place: _ } => {
+                        for target in return_.into_iter().chain(cleanup.into_iter()) {
+                            self.label_block(target, current_task);
+                        }
+                    }
+                    mir::TerminatorEdges::SwitchInt { targets, discr: _ } => {
+                        for target in targets.all_targets() {
+                            self.label_block(*target, current_task);
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+// 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.

From 997cf111cd4fabd3c743f2f7f0f9c7a40f28920d Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Mon, 1 Apr 2024 14:42:38 -0400
Subject: [PATCH 02/19] Update initialized-variable analysis to consider syncs

This commit extends mark_cilk_tasks::TaskTree with useful methods for
initialized variable analysis (mostly ways to observe the state of a
TaskTree). It primarily extends the analyses of initialized variables
to consider syncs as initializing all variables that are initialized
at reattachment points.

One part I'm not sure of: we want the number of initialized variables to
decrease when we merge in DefinitelyInitializedVariables if the reattaches
are from the same task, and increase if they're not from the same task.
---
 .../src/impls/initialized.rs                  | 129 +++++++++++++++++-
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs |  18 +++
 2 files changed, 142 insertions(+), 5 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
index 720515f262db8..ce969c27fbe03 100644
--- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs
+++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
@@ -3,14 +3,14 @@ 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::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 +53,35 @@ 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: mark_cilk_tasks::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<ChunkedBitSet<MovePathIndex>>,
+    >,
+}
+
+fn task_tree_of_body<'a, 'tcx>(body: &'a Body<'tcx>) -> mark_cilk_tasks::TaskTree {
+    use rustc_middle::mir::visit::Visitor;
+    let mut task_tree = mark_cilk_tasks::TaskTree::new();
+    task_tree.visit_body(body);
+    task_tree.validate();
+    task_tree
 }
 
 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: task_tree_of_body(body),
+            state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
+        }
     }
 
     pub fn skipping_unreachable_unwind(mut self) -> Self {
@@ -130,16 +154,25 @@ pub struct MaybeUninitializedPlaces<'a, 'tcx> {
 
     mark_inactive_variants_as_uninit: bool,
     skip_unreachable_unwind: BitSet<mir::BasicBlock>,
+
+    /// See [MaybeInitializedPlaces::task_tree].
+    task_tree: mark_cilk_tasks::TaskTree,
+    /// See [MaybeInitializedPlaces::state_at_last_locations]
+    state_at_last_locations:
+        rustc_data_structures::fx::FxHashMap<Location, ChunkedBitSet<MovePathIndex>>,
 }
 
 impl<'a, 'tcx> MaybeUninitializedPlaces<'a, 'tcx> {
     pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self {
+        // TODO(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: task_tree_of_body(body),
+            state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
         }
     }
 
@@ -205,11 +238,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: mark_cilk_tasks::TaskTree,
+    state_at_last_locations:
+        rustc_data_structures::fx::FxHashMap<Location, lattice::Dual<BitSet<MovePathIndex>>>,
 }
 
 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: task_tree_of_body(body),
+            state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
+        }
     }
 }
 
@@ -251,11 +292,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: mark_cilk_tasks::TaskTree,
+    state_at_last_locations:
+        rustc_data_structures::fx::FxHashMap<Location, ChunkedBitSet<InitIndex>>,
 }
 
 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: task_tree_of_body(body),
+            state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
+        }
     }
 }
 
@@ -374,6 +423,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 +564,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(_)));
@@ -617,6 +703,26 @@ 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);
+            self.task_tree
+                .children_last_locations(task)
+                .filter_map(|location| self.state_at_last_locations.get(&location))
+                .for_each(|state| {
+                    // 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. This is a SOUNDNESS problem that we should definitely fix.
+
+                    // Since this lattice has a join operator of intersection and a set bit implies an initialized value, we want
+                    // to use union here. We want to get 'lower' in the lattice so we use meet (which will also be union).
+                    use crate::framework::lattice::MeetSemiLattice;
+                    trans.meet(state);
+                });
+        }
         terminator.edges()
     }
 
@@ -714,6 +820,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/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index edb5d5c00d0d5..043e28d891a1e 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -70,12 +70,30 @@ impl TaskTree {
         );
     }
 
+    /// Create a new TaskTree.
     pub fn new() -> Self {
         Self {
             tasks: IndexVec::new(),
             basic_blocks: rustc_data_structures::fx::FxHashMap::default(),
         }
     }
+
+    fn task(&self, block: BasicBlock) -> Option<Task> {
+        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: mir::Location) -> Task {
+        self.task(location.block).expect("expected block to have a task!")
+    }
+
+    /// Get the last locations of the children of this task.
+    pub fn children_last_locations(&self, task: Task) -> impl Iterator<Item = mir::Location> + '_ {
+        self.tasks[task]
+            .children
+            .iter()
+            .flat_map(move |&child| self.tasks[child].last_locations.iter().copied())
+    }
 }
 
 impl<'tcx> Visitor<'tcx> for TaskTree {

From 8f763d961568688e97b769ad4787e2aa4a6db0e5 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 10:23:33 -0400
Subject: [PATCH 03/19] Fix DefinitelyInitializedVariables to merge state
 correctly

This commit changes DefinitelyInitializedVariables to merge the
dataflow state at the places a task exits via join (intersection),
which makes sense because it reduces the number of initialized
places. We then use meet (union) to merge the initialized variable
state after all of the tasks are done at the sync. The bug with
syncing conditional spawns is still possible.
---
 .../src/impls/initialized.rs                  | 41 ++++++++++++++-----
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs | 11 +++++
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
index ce969c27fbe03..9af0d036bfe46 100644
--- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs
+++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
@@ -703,26 +703,45 @@ 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_last_locations(task)
-                .filter_map(|location| self.state_at_last_locations.get(&location))
+                .last_locations_by_child(task)
+                .map(|(_task, locations)| {
+                    // 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()));
+                    let state_exiting_task = locations
+                        .iter()
+                        .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
+                        });
+                    state_exiting_task
+                })
                 .for_each(|state| {
-                    // 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. This is a SOUNDNESS problem that we should definitely fix.
-
-                    // Since this lattice has a join operator of intersection and a set bit implies an initialized value, we want
-                    // to use union here. We want to get 'lower' in the lattice so we use meet (which will also be union).
+                    // 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);
+                    trans.meet(&state);
                 });
         }
+
         terminator.edges()
     }
 
diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index 043e28d891a1e..9056a28b3c41a 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -94,6 +94,17 @@ impl TaskTree {
             .iter()
             .flat_map(move |&child| self.tasks[child].last_locations.iter().copied())
     }
+
+    /// Get an iterator over child tasks and their last locations.
+    pub fn last_locations_by_child(
+        &self,
+        task: Task,
+    ) -> impl Iterator<Item = (Task, &[mir::Location])> + '_ {
+        self.tasks[task]
+            .children
+            .iter()
+            .map(|&child| (child, self.tasks[child].last_locations.as_ref()))
+    }
 }
 
 impl<'tcx> Visitor<'tcx> for TaskTree {

From 000993b4fa770fdb3e0a683820e6dc47b2fa0c08 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 12:02:33 -0400
Subject: [PATCH 04/19] Refactor DefinitelyInitializedPlaces::terminator_effect

Pulls the way we merge dataflow state within a task into a helper function.
This makes the function easier-to-read. We also change the public API of
mark_cilk_tasks since last_locations_by_child was hard to compose.
---
 .../src/impls/initialized.rs                  | 50 +++++++++++--------
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs | 25 ++++------
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
index 9af0d036bfe46..19c100751836e 100644
--- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs
+++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
@@ -676,6 +676,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,
+    ) -> <Self as AnalysisDomain<'tcx>>::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;
 
@@ -713,33 +737,15 @@ impl<'tcx> GenKillAnalysis<'tcx> for DefinitelyInitializedPlaces<'_, 'tcx> {
             // 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
-                .last_locations_by_child(task)
-                .map(|(_task, locations)| {
-                    // 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()));
-                    let state_exiting_task = locations
-                        .iter()
-                        .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
-                        });
-                    state_exiting_task
-                })
-                .for_each(|state| {
+            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()
diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index 9056a28b3c41a..fe44c53d39ad9 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -87,23 +87,20 @@ impl TaskTree {
         self.task(location.block).expect("expected block to have a task!")
     }
 
-    /// Get the last locations of the children of this 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<Item = mir::Location> + '_ {
-        self.tasks[task]
-            .children
-            .iter()
-            .flat_map(move |&child| self.tasks[child].last_locations.iter().copied())
+        self.children(task).flat_map(move |child| self.last_locations(child))
     }
 
-    /// Get an iterator over child tasks and their last locations.
-    pub fn last_locations_by_child(
-        &self,
-        task: Task,
-    ) -> impl Iterator<Item = (Task, &[mir::Location])> + '_ {
-        self.tasks[task]
-            .children
-            .iter()
-            .map(|&child| (child, self.tasks[child].last_locations.as_ref()))
+    /// Get the children of this task, panicking if it doesn't exist.
+    pub fn children(&self, task: Task) -> impl Iterator<Item = Task> + '_ {
+        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<Item = mir::Location> + '_ {
+        self.tasks[task].last_locations.iter().copied()
     }
 }
 

From bb763f0420410f6bd733e7f723b0bb3754aaff3c Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 12:11:59 -0400
Subject: [PATCH 05/19] Appease tidy script

---
 compiler/rustc_borrowck/src/polonius/loan_invalidations.rs   | 4 ++--
 compiler/rustc_mir_dataflow/src/impls/initialized.rs         | 2 +-
 compiler/rustc_monomorphize/src/collector.rs                 | 2 +-
 tests/ui/cilk/borrows_dropped_after_sync.rs                  | 2 +-
 tests/ui/cilk/borrows_live_before_sync.rs                    | 4 ++--
 .../ui/cilk/proper_scoping_of_variables_in_spawned_block.rs  | 5 +++--
 tests/ui/cilk/require_sync.rs                                | 2 +-
 7 files changed, 11 insertions(+), 10 deletions(-)

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_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
index 19c100751836e..7e7cf8b137bef 100644
--- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs
+++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
@@ -164,7 +164,7 @@ pub struct MaybeUninitializedPlaces<'a, 'tcx> {
 
 impl<'a, 'tcx> MaybeUninitializedPlaces<'a, 'tcx> {
     pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self {
-        // TODO(jhilton): non-trivial work in constructor :(
+        // FIXME(jhilton): non-trivial work in constructor :(
         MaybeUninitializedPlaces {
             tcx,
             body,
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/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/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/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<T> is not Sync so we can't use it here. 
+    // Rc<T> is not Sync so we can't use it here.
     cilk_spawn { let x = Rc::clone(&x); x.replace_with(|n| *n + 1) };
 }

From 117f35e310835229b33da13cfa8c922130ea25f1 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 12:37:40 -0400
Subject: [PATCH 06/19] Use a preorder traversal rather than visitor to build
 TaskTree

When we used a visitor, we saw ICEs when building rustc. I think this is
because of the particular order of the visitor, but here we care about the
traversal order and only need to worry about basic block terminators.
A preorder traversal makes sense because we want to ensure that the only
block which has a new task constructed for it is the root of the Body,
and this is true in a preorder traversal as long as the Body is connected
(which seems to be a reasonable assumption).

If at some point the body is disconnected, we can allow reusing the default
task since the task we assign doesn't actually matter: the disconnected
portion of the graph (whichever one isn't reachable from the root) will
never be executed and should be removed by dead code elimination.
---
 .../src/impls/initialized.rs                  |  25 ++-
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs | 151 ++++++++++--------
 2 files changed, 89 insertions(+), 87 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
index 7e7cf8b137bef..62442d4a92d87 100644
--- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs
+++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs
@@ -5,6 +5,7 @@ use rustc_middle::ty::{self, TyCtxt};
 
 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;
@@ -54,7 +55,7 @@ pub struct MaybeInitializedPlaces<'a, 'tcx> {
     mdpe: &'a MoveDataParamEnv<'tcx>,
     skip_unreachable_unwind: bool,
     /// Maps basic blocks to the task they are part of.
-    task_tree: mark_cilk_tasks::TaskTree,
+    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<
@@ -63,14 +64,6 @@ pub struct MaybeInitializedPlaces<'a, 'tcx> {
     >,
 }
 
-fn task_tree_of_body<'a, 'tcx>(body: &'a Body<'tcx>) -> mark_cilk_tasks::TaskTree {
-    use rustc_middle::mir::visit::Visitor;
-    let mut task_tree = mark_cilk_tasks::TaskTree::new();
-    task_tree.visit_body(body);
-    task_tree.validate();
-    task_tree
-}
-
 impl<'a, 'tcx> MaybeInitializedPlaces<'a, 'tcx> {
     pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self {
         // FIXME(jhilton): I don't like that this constructor does non-trivial work. Make the task tree a parameter?
@@ -79,7 +72,7 @@ impl<'a, 'tcx> MaybeInitializedPlaces<'a, 'tcx> {
             body,
             mdpe,
             skip_unreachable_unwind: false,
-            task_tree: task_tree_of_body(body),
+            task_tree: TaskTree::from_body(body),
             state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
         }
     }
@@ -156,7 +149,7 @@ pub struct MaybeUninitializedPlaces<'a, 'tcx> {
     skip_unreachable_unwind: BitSet<mir::BasicBlock>,
 
     /// See [MaybeInitializedPlaces::task_tree].
-    task_tree: mark_cilk_tasks::TaskTree,
+    task_tree: TaskTree,
     /// See [MaybeInitializedPlaces::state_at_last_locations]
     state_at_last_locations:
         rustc_data_structures::fx::FxHashMap<Location, ChunkedBitSet<MovePathIndex>>,
@@ -171,7 +164,7 @@ impl<'a, 'tcx> MaybeUninitializedPlaces<'a, 'tcx> {
             mdpe,
             mark_inactive_variants_as_uninit: false,
             skip_unreachable_unwind: BitSet::new_empty(body.basic_blocks.len()),
-            task_tree: task_tree_of_body(body),
+            task_tree: TaskTree::from_body(body),
             state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
         }
     }
@@ -238,7 +231,7 @@ impl<'a, 'tcx> HasMoveData<'tcx> for MaybeUninitializedPlaces<'a, 'tcx> {
 pub struct DefinitelyInitializedPlaces<'a, 'tcx> {
     body: &'a Body<'tcx>,
     mdpe: &'a MoveDataParamEnv<'tcx>,
-    task_tree: mark_cilk_tasks::TaskTree,
+    task_tree: TaskTree,
     state_at_last_locations:
         rustc_data_structures::fx::FxHashMap<Location, lattice::Dual<BitSet<MovePathIndex>>>,
 }
@@ -248,7 +241,7 @@ impl<'a, 'tcx> DefinitelyInitializedPlaces<'a, 'tcx> {
         DefinitelyInitializedPlaces {
             body,
             mdpe,
-            task_tree: task_tree_of_body(body),
+            task_tree: TaskTree::from_body(body),
             state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
         }
     }
@@ -292,7 +285,7 @@ impl<'a, 'tcx> HasMoveData<'tcx> for DefinitelyInitializedPlaces<'a, 'tcx> {
 pub struct EverInitializedPlaces<'a, 'tcx> {
     body: &'a Body<'tcx>,
     mdpe: &'a MoveDataParamEnv<'tcx>,
-    task_tree: mark_cilk_tasks::TaskTree,
+    task_tree: TaskTree,
     state_at_last_locations:
         rustc_data_structures::fx::FxHashMap<Location, ChunkedBitSet<InitIndex>>,
 }
@@ -302,7 +295,7 @@ impl<'a, 'tcx> EverInitializedPlaces<'a, 'tcx> {
         EverInitializedPlaces {
             body,
             mdpe,
-            task_tree: task_tree_of_body(body),
+            task_tree: TaskTree::from_body(body),
             state_at_last_locations: rustc_data_structures::fx::FxHashMap::default(),
         }
     }
diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index fe44c53d39ad9..6350240e01b6f 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -1,6 +1,6 @@
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::IndexVec;
-use rustc_middle::mir::{self, visit::Visitor, BasicBlock};
+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
@@ -16,7 +16,7 @@ struct TaskData {
     /// Represents all child tasks that may be spawned by this task.
     pub children: Vec<Task>,
     /// Represents all locations which this task might end at where control will not be returned to it.
-    pub last_locations: Vec<mir::Location>,
+    pub last_locations: Vec<Location>,
 }
 
 pub struct TaskTree {
@@ -34,7 +34,11 @@ impl TaskTree {
             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!")
+                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,
@@ -83,12 +87,12 @@ impl TaskTree {
     }
 
     /// Get the task for the given location, panicking if it doesn't exist.
-    pub fn expect_task(&self, location: mir::Location) -> Task {
+    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<Item = mir::Location> + '_ {
+    pub fn children_last_locations(&self, task: Task) -> impl Iterator<Item = Location> + '_ {
         self.children(task).flat_map(move |child| self.last_locations(child))
     }
 
@@ -99,83 +103,88 @@ impl TaskTree {
 
     /// 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<Item = mir::Location> + '_ {
+    pub fn last_locations(&self, task: Task) -> impl Iterator<Item = Location> + '_ {
         self.tasks[task].last_locations.iter().copied()
     }
-}
 
-impl<'tcx> Visitor<'tcx> for TaskTree {
-    fn visit_terminator(
-        &mut self,
-        terminator: &rustc_middle::mir::Terminator<'tcx>,
-        location: rustc_middle::mir::Location,
-    ) {
-        // If we see a terminator, we want to mark the reachable blocks as being part of
-        // the current task, unless this is a Detach, in which case the spawned task is part
-        // of a new task. On a reattach, the task should be marked as the parent of whatever task
-        // this basic block is part of.
-
-        // This makes sense because we expect it to only happen once. When we finalize the analysis, we'll make sure
-        // that there's exactly one task with no parent (an orphan task).
-        let current_task = *self.basic_blocks.entry(location.block).or_insert_with(|| {
-            self.tasks.push(TaskData { parent: None, children: vec![], last_locations: vec![] })
-        });
-        match terminator.kind {
-            mir::TerminatorKind::Detach { spawned_task, continuation } => {
-                self.label_block(continuation, current_task);
-                let new_task = self.tasks.push(TaskData {
-                    parent: Some(current_task),
+    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::new();
+        for (block, block_data) in mir::traversal::preorder(body) {
+            let location = Location { block, statement_index: block_data.statements.len() };
+            let terminator = block_data.terminator();
+            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![],
-                });
-                self.tasks[current_task].children.push(new_task);
-                self.label_block(spawned_task, new_task);
-            }
-            mir::TerminatorKind::Reattach { continuation } => {
-                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 old 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!");
-                self.label_block(continuation, parent);
-
-                debug_assert!(
-                    self.tasks[parent].children.contains(&current_task),
-                    "the current task should be a child of the task being reattached to!"
-                );
-            }
-            _ => {
-                // For all other terminators, we want to mark all targets as children of the current task.
-                // This might have the wrong semantics with panics and unwinding? Hopefully sync insertion
-                // can make that a nonissue.
-                match terminator.edges() {
-                    mir::TerminatorEdges::None => {
-                        // No targets, nothing to do
-                    }
-                    mir::TerminatorEdges::Single(target) => {
-                        self.label_block(target, current_task);
-                    }
-                    mir::TerminatorEdges::Double(target1, target2) => {
-                        self.label_block(target1, current_task);
-                        self.label_block(target2, current_task);
-                    }
-                    mir::TerminatorEdges::AssignOnReturn { return_, cleanup, place: _ } => {
-                        for target in return_.into_iter().chain(cleanup.into_iter()) {
-                            self.label_block(target, current_task);
+                })
+            });
+
+            match terminator.kind {
+                mir::TerminatorKind::Detach { spawned_task, continuation } => {
+                    task_tree.label_block(continuation, current_task);
+                    let new_task = task_tree.tasks.push(TaskData {
+                        parent: Some(current_task),
+                        children: vec![],
+                        last_locations: vec![],
+                    });
+                    task_tree.tasks[current_task].children.push(new_task);
+                    task_tree.label_block(spawned_task, new_task);
+                }
+                mir::TerminatorKind::Reattach { continuation } => {
+                    let current_task_data = &mut task_tree.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!");
+                    task_tree.label_block(continuation, parent);
+
+                    debug_assert!(
+                        task_tree.tasks[parent].children.contains(&current_task),
+                        "the current task should be a child of the task being reattached to!"
+                    );
+                }
+                _ => {
+                    // For all other terminators, we want to mark all targets as children of the current task.
+                    // This might have the wrong semantics with panics and unwinding? Hopefully sync insertion
+                    // can make that a nonissue.
+                    match terminator.edges() {
+                        mir::TerminatorEdges::None => {
+                            // No targets, nothing to do
                         }
-                    }
-                    mir::TerminatorEdges::SwitchInt { targets, discr: _ } => {
-                        for target in targets.all_targets() {
-                            self.label_block(*target, current_task);
+                        mir::TerminatorEdges::Single(target) => {
+                            task_tree.label_block(target, current_task);
+                        }
+                        mir::TerminatorEdges::Double(target1, target2) => {
+                            task_tree.label_block(target1, current_task);
+                            task_tree.label_block(target2, current_task);
+                        }
+                        mir::TerminatorEdges::AssignOnReturn { return_, cleanup, place: _ } => {
+                            for target in return_.into_iter().chain(cleanup.into_iter()) {
+                                task_tree.label_block(target, current_task);
+                            }
+                        }
+                        mir::TerminatorEdges::SwitchInt { targets, discr: _ } => {
+                            for target in targets.all_targets() {
+                                task_tree.label_block(*target, current_task);
+                            }
                         }
                     }
                 }
             }
         }
+        task_tree.validate();
+        task_tree
     }
 }
 

From bbe3d4e3d282bf09c89acb339635bee839b2c230 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 12:47:26 -0400
Subject: [PATCH 07/19] Minor nits about variable placement in
 TaskTree::from_body

---
 compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index 6350240e01b6f..c0c2d16bdc33e 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -112,8 +112,6 @@ impl TaskTree {
         // We need to know that all ancestors of a block are visited before the block itself.
         let mut task_tree = Self::new();
         for (block, block_data) in mir::traversal::preorder(body) {
-            let location = Location { block, statement_index: block_data.statements.len() };
-            let terminator = block_data.terminator();
             let current_task = *task_tree.basic_blocks.entry(block).or_insert_with(|| {
                 assert!(
                     task_tree.tasks.is_empty(),
@@ -126,6 +124,9 @@ impl TaskTree {
                 })
             });
 
+            // 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.label_block(continuation, current_task);

From a36f4ba215e1a19fb503e601bfe1cd50f43d6452 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 13:06:54 -0400
Subject: [PATCH 08/19] Refactor TaskTree::from_body into helpers

Changes `from_body` to use helpers for handling each kind of terminator.
This lets us use better names and makes from_body a little easier to read
at a glance.
---
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs | 114 +++++++++++-------
 1 file changed, 69 insertions(+), 45 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index c0c2d16bdc33e..f6d672c7bfce4 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -107,6 +107,72 @@ impl TaskTree {
         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(&current_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_edges_to_current_task(&mut self, current_task: Task, terminator: &mir::Terminator<'_>) {
+        match terminator.edges() {
+            mir::TerminatorEdges::None => {
+                // No targets, nothing to do
+            }
+            mir::TerminatorEdges::Single(target) => {
+                self.label_block(target, current_task);
+            }
+            mir::TerminatorEdges::Double(target1, target2) => {
+                self.label_block(target1, current_task);
+                self.label_block(target2, current_task);
+            }
+            mir::TerminatorEdges::AssignOnReturn { return_, cleanup, place: _ } => {
+                for target in return_.into_iter().chain(cleanup.into_iter()) {
+                    self.label_block(target, current_task);
+                }
+            }
+            mir::TerminatorEdges::SwitchInt { targets, discr: _ } => {
+                for target in targets.all_targets() {
+                    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.
@@ -129,58 +195,16 @@ impl TaskTree {
             let terminator = block_data.terminator();
             match terminator.kind {
                 mir::TerminatorKind::Detach { spawned_task, continuation } => {
-                    task_tree.label_block(continuation, current_task);
-                    let new_task = task_tree.tasks.push(TaskData {
-                        parent: Some(current_task),
-                        children: vec![],
-                        last_locations: vec![],
-                    });
-                    task_tree.tasks[current_task].children.push(new_task);
-                    task_tree.label_block(spawned_task, new_task);
+                    task_tree.detach_at(current_task, spawned_task, continuation);
                 }
                 mir::TerminatorKind::Reattach { continuation } => {
-                    let current_task_data = &mut task_tree.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!");
-                    task_tree.label_block(continuation, parent);
-
-                    debug_assert!(
-                        task_tree.tasks[parent].children.contains(&current_task),
-                        "the current task should be a child of the task being reattached to!"
-                    );
+                    task_tree.reattach_at(location, current_task, continuation);
                 }
                 _ => {
                     // For all other terminators, we want to mark all targets as children of the current task.
                     // This might have the wrong semantics with panics and unwinding? Hopefully sync insertion
                     // can make that a nonissue.
-                    match terminator.edges() {
-                        mir::TerminatorEdges::None => {
-                            // No targets, nothing to do
-                        }
-                        mir::TerminatorEdges::Single(target) => {
-                            task_tree.label_block(target, current_task);
-                        }
-                        mir::TerminatorEdges::Double(target1, target2) => {
-                            task_tree.label_block(target1, current_task);
-                            task_tree.label_block(target2, current_task);
-                        }
-                        mir::TerminatorEdges::AssignOnReturn { return_, cleanup, place: _ } => {
-                            for target in return_.into_iter().chain(cleanup.into_iter()) {
-                                task_tree.label_block(target, current_task);
-                            }
-                        }
-                        mir::TerminatorEdges::SwitchInt { targets, discr: _ } => {
-                            for target in targets.all_targets() {
-                                task_tree.label_block(*target, current_task);
-                            }
-                        }
-                    }
+                    task_tree.add_edges_to_current_task(current_task, terminator);
                 }
             }
         }

From d21f80649621fa79e503c38c98dcea555e724381 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Tue, 2 Apr 2024 13:19:10 -0400
Subject: [PATCH 09/19] Update test expectation line numbers

---
 .../cilk/proper_scoping_of_variables_in_spawned_block.stderr  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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 };
    |                              ^

From bade5973c34758052a15b8ecc5c85ec6ba3aca9e Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Thu, 4 Apr 2024 12:28:13 -0400
Subject: [PATCH 10/19] Implement Extend<T> for WorkQueue<T>

---
 compiler/rustc_data_structures/src/work_queue.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

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<T: Idx> WorkQueue<T> {
         }
     }
 }
+
+impl<T: Idx> Extend<T> for WorkQueue<T> {
+    fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) {
+        for element in iter {
+            self.insert(element);
+        }
+    }
+}

From 2a7d55391dfc3563024d338d5a64368426a9ade4 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Thu, 4 Apr 2024 13:31:22 -0400
Subject: [PATCH 11/19] Avoid labeling basic blocks in unwind subgraph with a
 task

We now do not label basic blocks in the unwind subgraph with a task.
This is because the cleanup blocks in the unwind subgraph are reachable from
non-cleanup blocks when they unwind. This would lead to labeling unwind blocks
with many possible tasks, which breaks our invariant that blocks have exactly
one task.
---
 .../rustc_mir_dataflow/src/mark_cilk_tasks.rs | 103 ++++++++++++------
 1 file changed, 70 insertions(+), 33 deletions(-)

diff --git a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
index f6d672c7bfce4..778cf852173b1 100644
--- a/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
+++ b/compiler/rustc_mir_dataflow/src/mark_cilk_tasks.rs
@@ -1,4 +1,6 @@
 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};
 
@@ -22,6 +24,46 @@ struct TaskData {
 pub struct TaskTree {
     tasks: IndexVec<Task, TaskData>,
     basic_blocks: FxHashMap<BasicBlock, Task>,
+    unwind_subgraph: BitSet<BasicBlock>,
+    cleanup_blocks: BitSet<BasicBlock>,
+}
+
+fn cleanup_blocks(body: &mir::Body<'_>) -> BitSet<BasicBlock> {
+    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<BasicBlock> {
+    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 {
@@ -30,6 +72,12 @@ impl TaskTree {
     /// 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 =>
@@ -74,14 +122,6 @@ impl TaskTree {
         );
     }
 
-    /// Create a new TaskTree.
-    pub fn new() -> Self {
-        Self {
-            tasks: IndexVec::new(),
-            basic_blocks: rustc_data_structures::fx::FxHashMap::default(),
-        }
-    }
-
     fn task(&self, block: BasicBlock) -> Option<Task> {
         self.basic_blocks.get(&block).copied()
     }
@@ -147,37 +187,33 @@ impl TaskTree {
     }
 
     /// Modify this task tree by adding all blocks that the terminator can go to, to the current task.
-    fn add_edges_to_current_task(&mut self, current_task: Task, terminator: &mir::Terminator<'_>) {
-        match terminator.edges() {
-            mir::TerminatorEdges::None => {
-                // No targets, nothing to do
-            }
-            mir::TerminatorEdges::Single(target) => {
+    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);
             }
-            mir::TerminatorEdges::Double(target1, target2) => {
-                self.label_block(target1, current_task);
-                self.label_block(target2, current_task);
-            }
-            mir::TerminatorEdges::AssignOnReturn { return_, cleanup, place: _ } => {
-                for target in return_.into_iter().chain(cleanup.into_iter()) {
-                    self.label_block(target, current_task);
-                }
-            }
-            mir::TerminatorEdges::SwitchInt { targets, discr: _ } => {
-                for target in targets.all_targets() {
-                    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::new();
+        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(),
@@ -202,9 +238,10 @@ impl TaskTree {
                 }
                 _ => {
                     // For all other terminators, we want to mark all targets as children of the current task.
-                    // This might have the wrong semantics with panics and unwinding? Hopefully sync insertion
-                    // can make that a nonissue.
-                    task_tree.add_edges_to_current_task(current_task, terminator);
+                    // 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);
                 }
             }
         }

From 02233bcaf935a827deef0ba6999f50708b06d580 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Thu, 4 Apr 2024 14:09:30 -0400
Subject: [PATCH 12/19] Implement and test
 ChunkedBitSet::intersect(ChunkedBitSet)

---
 compiler/rustc_index/src/bit_set.rs       | 59 +++++++++++++++-
 compiler/rustc_index/src/bit_set/tests.rs | 86 +++++++++++++++++++++++
 2 files changed, 143 insertions(+), 2 deletions(-)

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<T: Idx> BitRelations<ChunkedBitSet<T>> for ChunkedBitSet<T> {
         unimplemented!("implement if/when necessary");
     }
 
-    fn intersect(&mut self, _other: &ChunkedBitSet<T>) -> bool {
-        unimplemented!("implement if/when necessary");
+    fn intersect(&mut self, other: &ChunkedBitSet<T>) -> 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<usize> = 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<usize> = Vec::new();
+    let vec2: Vec<usize> = (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<usize> = Vec::new();
+    let vec2: Vec<usize> = 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<usize> = (0..n).collect();
+    let vec2: Vec<usize> = (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<usize> = (0..n).collect();
+    let vec2: Vec<usize> = vec![0, 1, 2, 2010, 2047, 2099, 6000, 6002, 6004];
+    let vec_expected: Vec<usize> = 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<usize> = vec![0, 1, 2, 2010, 5, 10, 20];
+    let vec2: Vec<usize> = vec![0, 1, 2, 2010, 2047, 2099, 6000, 6002, 6004];
+    let vec_expected: Vec<usize> = 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<usize> = GrowableBitSet::with_capacity(65);

From 7013aac50fc75be737c3b3420b0d4cd262bba1f5 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 01:05:14 -0400
Subject: [PATCH 13/19] Update test expectations for correct semantics

---
 tests/ui/cilk/const_cilk_keywords.rs           |  2 +-
 tests/ui/cilk/const_cilk_keywords.stderr       | 12 ------------
 tests/ui/cilk/fib_block_recurse.rs             |  2 +-
 tests/ui/cilk/fib_block_recurse.stderr         | 12 ------------
 tests/ui/cilk/fib_block_recurse_no_sync.rs     |  3 +--
 tests/ui/cilk/fib_block_recurse_no_sync.stderr |  8 +++++---
 6 files changed, 8 insertions(+), 31 deletions(-)
 delete mode 100644 tests/ui/cilk/const_cilk_keywords.stderr
 delete mode 100644 tests/ui/cilk/fib_block_recurse.stderr

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/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
 

From b4ea04a4a562ba00a591897dee7545e8cf0843a0 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 01:06:30 -0400
Subject: [PATCH 14/19] Only inject FakeRead for when the RHS is not a
 CilkSpawn

Previously, the LHS of an assignment is always used by a FakeRead
for better diagnostics, since it's otherwise possible to create
variable that can't actually be used. The read makes those
initializations an error. However, the value is not available in
the case of a spawn until a sync, so we get this benefit anyways.
---
 compiler/rustc_mir_build/src/build/matches/mod.rs | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs
index 35f5a6bfac5f4..bbd45288844e9 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -566,9 +566,16 @@ 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);
+                let should_inject_fake_read = matches!(
+                    self.thir[initializer_id],
+                    Expr { kind: ExprKind::CilkSpawn { .. }, .. }
+                );
+
+                if should_inject_fake_read {
+                    // 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()

From b6301bc87ce9d1d4b29f3469f87b54049698600d Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 01:19:43 -0400
Subject: [PATCH 15/19] Add additional tests for sync in uninitialized variable
 checking

matching_on_spawned_expression tests that matching on an un-synced expression fails,
and fib_block_recurse_type_ascription checks that type ascription works the same way
as without type ascription.
---
 tests/ui/cilk/fib_block_recurse_type_ascription.rs | 14 ++++++++++++++
 tests/ui/cilk/matching_on_spawned_expression.rs    | 10 ++++++++++
 .../ui/cilk/matching_on_spawned_expression.stderr  | 13 +++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 tests/ui/cilk/fib_block_recurse_type_ascription.rs
 create mode 100644 tests/ui/cilk/matching_on_spawned_expression.rs
 create mode 100644 tests/ui/cilk/matching_on_spawned_expression.stderr

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<usize> = 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<usize> = 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`.

From 7bda8b669aeefed32852e352ce9a652c6b6c972d Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 01:21:24 -0400
Subject: [PATCH 16/19] Don't emit FakeRead when assigning the result of a
 spawn

---
 .../rustc_mir_build/src/build/matches/mod.rs  | 28 ++++++++++++-------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs
index bbd45288844e9..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,12 +577,7 @@ 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));
 
-                let should_inject_fake_read = matches!(
-                    self.thir[initializer_id],
-                    Expr { kind: ExprKind::CilkSpawn { .. }, .. }
-                );
-
-                if should_inject_fake_read {
+                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);
@@ -604,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);
 

From 8f26f0a1e0924574c9d63872013b8477bd28c9d1 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 02:09:58 -0400
Subject: [PATCH 17/19] Add test that an error occurs when we return from a
 spawned block

---
 .../error_when_value_returned_from_spawned_block.rs | 10 ++++++++++
 ...or_when_value_returned_from_spawned_block.stderr | 13 +++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 tests/ui/cilk/error_when_value_returned_from_spawned_block.rs
 create mode 100644 tests/ui/cilk/error_when_value_returned_from_spawned_block.stderr

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`.

From 1d410140be520286c0f41cf4fa18be67d0b30357 Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 02:17:24 -0400
Subject: [PATCH 18/19] Resolve outdated FIXME about AST liveness

---
 compiler/rustc_passes/src/liveness.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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(..)

From 67f46145e2dc1e9676256e0087c1065c9e6383fe Mon Sep 17 00:00:00 2001
From: jhilton <jhilton@mit.edu>
Date: Fri, 5 Apr 2024 02:34:47 -0400
Subject: [PATCH 19/19] Rename cilk feature gate test to follow convention

---
 tests/ui/feature-gates/{cilk.rs => feature-gate-cilk.rs}      | 0
 .../feature-gates/{cilk.stderr => feature-gate-cilk.stderr}   | 4 ++--
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename tests/ui/feature-gates/{cilk.rs => feature-gate-cilk.rs} (100%)
 rename tests/ui/feature-gates/{cilk.stderr => feature-gate-cilk.stderr} (89%)

diff --git a/tests/ui/feature-gates/cilk.rs b/tests/ui/feature-gates/feature-gate-cilk.rs
similarity index 100%
rename from tests/ui/feature-gates/cilk.rs
rename to tests/ui/feature-gates/feature-gate-cilk.rs
diff --git a/tests/ui/feature-gates/cilk.stderr b/tests/ui/feature-gates/feature-gate-cilk.stderr
similarity index 89%
rename from tests/ui/feature-gates/cilk.stderr
rename to tests/ui/feature-gates/feature-gate-cilk.stderr
index f176e63cc6a1b..04d7fb2d5b3fa 100644
--- a/tests/ui/feature-gates/cilk.stderr
+++ b/tests/ui/feature-gates/feature-gate-cilk.stderr
@@ -1,5 +1,5 @@
 error[E0658]: cilk keywords are experimental
-  --> $DIR/cilk.rs:2:16
+  --> $DIR/feature-gate-cilk.rs:2:16
    |
 LL |     cilk_spawn { 5 };
    |                ^^^^^
@@ -8,7 +8,7 @@ LL |     cilk_spawn { 5 };
    = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date
 
 error[E0658]: cilk keywords are experimental
-  --> $DIR/cilk.rs:3:5
+  --> $DIR/feature-gate-cilk.rs:3:5
    |
 LL |     cilk_sync;
    |     ^^^^^^^^^