Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feedback] per-edge dataflow #47664

Closed
wants to merge 8 commits into from
Closed

[feedback] per-edge dataflow #47664

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
68b9f0b
use `'_` to make clear that `BlockSets` contain references
nikomatsakis Dec 20, 2017
3f768fd
document that `sets.on_entry` is not *always* block entry
nikomatsakis Dec 20, 2017
af62153
convert `propagate_call_return` to take `sets` and be pure function
nikomatsakis Dec 20, 2017
3056861
s/&mut sets.on_entry/&sets.on_entry/
nikomatsakis Dec 20, 2017
7619704
introduce (unused) scratch buffer
nikomatsakis Dec 31, 2017
0285472
introduce helper for propagating bits across a list of edges
nikomatsakis Dec 31, 2017
5fe40f6
generalize `propagate_call_return` to `edge_effect`
nikomatsakis Dec 31, 2017
642815f
micro-optimize "anything changed?" check
nikomatsakis Dec 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions src/librustc_mir/dataflow/impls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ impl<'a, 'gcx, 'tcx> HasMoveData<'tcx> for EverInitializedLvals<'a, 'gcx, 'tcx>


impl<'a, 'gcx, 'tcx> MaybeInitializedLvals<'a, 'gcx, 'tcx> {
fn update_bits(sets: &mut BlockSets<MovePathIndex>, path: MovePathIndex,
fn update_bits(sets: &mut BlockSets<'_, MovePathIndex>, path: MovePathIndex,
state: DropFlagState)
{
match state {
Expand All @@ -303,7 +303,7 @@ impl<'a, 'gcx, 'tcx> MaybeInitializedLvals<'a, 'gcx, 'tcx> {
}

impl<'a, 'gcx, 'tcx> MaybeUninitializedLvals<'a, 'gcx, 'tcx> {
fn update_bits(sets: &mut BlockSets<MovePathIndex>, path: MovePathIndex,
fn update_bits(sets: &mut BlockSets<'_, MovePathIndex>, path: MovePathIndex,
state: DropFlagState)
{
match state {
Expand All @@ -314,7 +314,7 @@ impl<'a, 'gcx, 'tcx> MaybeUninitializedLvals<'a, 'gcx, 'tcx> {
}

impl<'a, 'gcx, 'tcx> DefinitelyInitializedLvals<'a, 'gcx, 'tcx> {
fn update_bits(sets: &mut BlockSets<MovePathIndex>, path: MovePathIndex,
fn update_bits(sets: &mut BlockSets<'_, MovePathIndex>, path: MovePathIndex,
state: DropFlagState)
{
match state {
Expand All @@ -341,7 +341,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MaybeInitializedLvals<'a, 'gcx, 'tcx> {
}

fn statement_effect(&self,
sets: &mut BlockSets<MovePathIndex>,
sets: &mut BlockSets<'_, MovePathIndex>,
location: Location)
{
drop_flag_effects_for_location(
Expand All @@ -352,7 +352,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MaybeInitializedLvals<'a, 'gcx, 'tcx> {
}

fn terminator_effect(&self,
sets: &mut BlockSets<MovePathIndex>,
sets: &mut BlockSets<'_, MovePathIndex>,
location: Location)
{
drop_flag_effects_for_location(
Expand Down Expand Up @@ -396,7 +396,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MaybeUninitializedLvals<'a, 'gcx, 'tcx> {
}

fn statement_effect(&self,
sets: &mut BlockSets<MovePathIndex>,
sets: &mut BlockSets<'_, MovePathIndex>,
location: Location)
{
drop_flag_effects_for_location(
Expand All @@ -407,7 +407,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MaybeUninitializedLvals<'a, 'gcx, 'tcx> {
}

fn terminator_effect(&self,
sets: &mut BlockSets<MovePathIndex>,
sets: &mut BlockSets<'_, MovePathIndex>,
location: Location)
{
drop_flag_effects_for_location(
Expand Down Expand Up @@ -450,7 +450,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for DefinitelyInitializedLvals<'a, 'gcx, 'tcx
}

fn statement_effect(&self,
sets: &mut BlockSets<MovePathIndex>,
sets: &mut BlockSets<'_, MovePathIndex>,
location: Location)
{
drop_flag_effects_for_location(
Expand All @@ -461,7 +461,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for DefinitelyInitializedLvals<'a, 'gcx, 'tcx
}

fn terminator_effect(&self,
sets: &mut BlockSets<MovePathIndex>,
sets: &mut BlockSets<'_, MovePathIndex>,
location: Location)
{
drop_flag_effects_for_location(
Expand Down Expand Up @@ -497,7 +497,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> {
}

fn statement_effect(&self,
sets: &mut BlockSets<MoveOutIndex>,
sets: &mut BlockSets<'_, MoveOutIndex>,
location: Location) {
let (tcx, mir, move_data) = (self.tcx, self.mir, self.move_data());
let stmt = &mir[location.block].statements[location.statement_index];
Expand Down Expand Up @@ -525,7 +525,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> {
}

fn terminator_effect(&self,
sets: &mut BlockSets<MoveOutIndex>,
sets: &mut BlockSets<'_, MoveOutIndex>,
location: Location)
{
let (tcx, mir, move_data) = (self.tcx, self.mir, self.move_data());
Expand All @@ -542,7 +542,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> {
}

fn propagate_call_return(&self,
in_out: &mut IdxSet<MoveOutIndex>,
sets: &mut BlockSets<'_, MoveOutIndex>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment may be made irrelevant by later commits in the series, but I don't think it was correct to replace IdxSet with BlockSets here.

IIRC, BlockSets is a triple of sets associated with the block currently being processed. But fn propagate_call_return needs to modify the successor block for the call terminator, which is why we pass in the whole in_out here and not just the three sets for the current block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think a commit further down the line did indeed make this feedback irrelevant. If it was ever relevant in the first place, which I am no longer certain of; the IdxSet here is not for the whole universe of blocks anyway...)

_call_bb: mir::BasicBlock,
_dest_bb: mir::BasicBlock,
dest_place: &mir::Place) {
Expand Down Expand Up @@ -575,7 +575,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> {
}

fn statement_effect(&self,
sets: &mut BlockSets<InitIndex>,
sets: &mut BlockSets<'_, InitIndex>,
location: Location) {
let (_, mir, move_data) = (self.tcx, self.mir, self.move_data());
let stmt = &mir[location.block].statements[location.statement_index];
Expand Down Expand Up @@ -622,7 +622,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> {
}

fn terminator_effect(&self,
sets: &mut BlockSets<InitIndex>,
sets: &mut BlockSets<'_, InitIndex>,
location: Location)
{
let (mir, move_data) = (self.mir, self.move_data());
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/dataflow/impls/storage_liveness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl<'a, 'tcx> BitDenotation for MaybeStorageLive<'a, 'tcx> {
}

fn propagate_call_return(&self,
_in_out: &mut IdxSet<Local>,
_sets: &mut BlockSets<'_, Local>,
_call_bb: mir::BasicBlock,
_dest_bb: mir::BasicBlock,
_dest_place: &mir::Place) {
Expand Down
13 changes: 6 additions & 7 deletions src/librustc_mir/dataflow/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,11 +612,10 @@ pub trait BitDenotation: BitwiseOperator {
/// `sets.on_entry` to that local clone into `statement_effect` and
/// `terminator_effect`).
///
/// When its false, no local clone is constucted; instead a
/// reference directly into `on_entry` is passed along via
/// `sets.on_entry` instead, which represents the flow state at
/// the block's start, not necessarily the state immediately prior
/// to the statement/terminator under analysis.
/// When its false, no local clone is constucted; instead it is
/// undefined what `on_entry` points to (in practice, it will
/// frequently be a reference the flow state at the block's start,
/// but you should not rely on that).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good change (in that no one should be or have been using the stronger guarantee in the earlier version of the comment). But I am curious: Did you actually concoct some scenario that invalidated the comment's stronger claim? Or are you figuring that we're better off not defining the semantics in this case?

///
/// In either case, the passed reference is mutable; but this is a
/// wart from using the `BlockSets` type in the API; the intention
Expand Down Expand Up @@ -674,7 +673,7 @@ pub trait BitDenotation: BitwiseOperator {
/// `bb_data` is the sequence of statements identified by `bb` in
/// the MIR.
fn statement_effect(&self,
sets: &mut BlockSets<Self::Idx>,
sets: &mut BlockSets<'_, Self::Idx>,
location: Location);

/// Similar to `terminator_effect`, except it applies
Expand Down Expand Up @@ -703,7 +702,7 @@ pub trait BitDenotation: BitwiseOperator {
/// The effects applied here cannot depend on which branch the
/// terminator took.
fn terminator_effect(&self,
sets: &mut BlockSets<Self::Idx>,
sets: &mut BlockSets<'_, Self::Idx>,
location: Location);

/// Mutates the block-sets according to the (flow-dependent)
Expand Down