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.

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

Revert propagation of drop-live information from Polonius #125652

Merged
merged 3 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 3 additions & 8 deletions compiler/rustc_borrowck/src/type_check/liveness/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use std::rc::Rc;
use crate::{
constraints::OutlivesConstraintSet,
facts::{AllFacts, AllFactsExt},
location::LocationTable,
region_infer::values::LivenessValues,
universal_regions::UniversalRegions,
};
Expand All @@ -39,7 +38,6 @@ pub(super) fn generate<'mir, 'tcx>(
elements: &Rc<DenseLocationMap>,
flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>,
move_data: &MoveData<'tcx>,
location_table: &LocationTable,
use_polonius: bool,
) {
debug!("liveness::generate");
Expand All @@ -53,11 +51,9 @@ pub(super) fn generate<'mir, 'tcx>(
compute_relevant_live_locals(typeck.tcx(), &free_regions, body);
let facts_enabled = use_polonius || AllFacts::enabled(typeck.tcx());

let polonius_drop_used = facts_enabled.then(|| {
let mut drop_used = Vec::new();
polonius::populate_access_facts(typeck, body, location_table, move_data, &mut drop_used);
drop_used
});
if facts_enabled {
polonius::populate_access_facts(typeck, body, move_data);
};

trace::trace(
typeck,
Expand All @@ -67,7 +63,6 @@ pub(super) fn generate<'mir, 'tcx>(
move_data,
relevant_live_locals,
boring_locals,
polonius_drop_used,
);

// Mark regions that should be live where they appear within rvalues or within a call: like
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_borrowck/src/type_check/liveness/polonius.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,10 @@ impl<'a, 'tcx> Visitor<'tcx> for UseFactsExtractor<'a, 'tcx> {
pub(super) fn populate_access_facts<'a, 'tcx>(
typeck: &mut TypeChecker<'a, 'tcx>,
body: &Body<'tcx>,
location_table: &LocationTable,
move_data: &MoveData<'tcx>,
//FIXME: this is not mutated, but expected to be modified as
// out param, bug?
dropped_at: &mut Vec<(Local, Location)>,
) {
debug!("populate_access_facts()");
let location_table = typeck.borrowck_context.location_table;

if let Some(facts) = typeck.borrowck_context.all_facts.as_mut() {
let mut extractor = UseFactsExtractor {
Expand All @@ -104,10 +101,6 @@ pub(super) fn populate_access_facts<'a, 'tcx>(
};
extractor.visit_body(body);

facts.var_dropped_at.extend(
dropped_at.iter().map(|&(local, location)| (local, location_table.mid_index(location))),
);

for (local, local_decl) in body.local_decls.iter_enumerated() {
debug!(
"add use_of_var_derefs_origin facts - local={:?}, type={:?}",
Expand Down
35 changes: 25 additions & 10 deletions compiler/rustc_borrowck/src/type_check/liveness/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use rustc_mir_dataflow::impls::MaybeInitializedPlaces;
use rustc_mir_dataflow::move_paths::{HasMoveData, MoveData, MovePathIndex};
use rustc_mir_dataflow::ResultsCursor;

use crate::location::RichLocation;
use crate::{
region_infer::values::{self, LiveLoans},
type_check::liveness::local_use_map::LocalUseMap,
Expand Down Expand Up @@ -46,7 +47,6 @@ pub(super) fn trace<'mir, 'tcx>(
move_data: &MoveData<'tcx>,
relevant_live_locals: Vec<Local>,
boring_locals: Vec<Local>,
polonius_drop_used: Option<Vec<(Local, Location)>>,
) {
let local_use_map = &LocalUseMap::build(&relevant_live_locals, elements, body);

Expand Down Expand Up @@ -93,9 +93,7 @@ pub(super) fn trace<'mir, 'tcx>(

let mut results = LivenessResults::new(cx);

if let Some(drop_used) = polonius_drop_used {
results.add_extra_drop_facts(drop_used, relevant_live_locals.iter().copied().collect())
}
results.add_extra_drop_facts(&relevant_live_locals);

results.compute_for_all_locals(relevant_live_locals);

Expand Down Expand Up @@ -218,21 +216,38 @@ impl<'me, 'typeck, 'flow, 'tcx> LivenessResults<'me, 'typeck, 'flow, 'tcx> {
///
/// Add facts for all locals with free regions, since regions may outlive
/// the function body only at certain nodes in the CFG.
fn add_extra_drop_facts(
&mut self,
drop_used: Vec<(Local, Location)>,
relevant_live_locals: FxIndexSet<Local>,
) {
fn add_extra_drop_facts(&mut self, relevant_live_locals: &[Local]) -> Option<()> {
let drop_used = self
.cx
.typeck
.borrowck_context
.all_facts
.as_ref()
.map(|facts| facts.var_dropped_at.clone())?;
Copy link
Member

Choose a reason for hiding this comment

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

drive-by question: how big is this vec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I wrote my master’s thesis drop-uses weren’t super common so I’d say small ish, but I’d like to remove this clone if possible.

Of course I had a dog walk idea how to do that just in time for it to get merged before I’d implemented it but I’ll look into it next time I need something to pass the time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should add that the clone happened in the original version too, except not as a clone but as a for each insert.


let relevant_live_locals: FxIndexSet<_> = relevant_live_locals.iter().copied().collect();

let locations = IntervalSet::new(self.cx.elements.num_points());

for (local, location) in drop_used {
for (local, location_index) in drop_used {
if !relevant_live_locals.contains(&local) {
let local_ty = self.cx.body.local_decls[local].ty;
if local_ty.has_free_regions() {
let location = match self
.cx
.typeck
.borrowck_context
.location_table
.to_location(location_index)
{
RichLocation::Start(l) => l,
RichLocation::Mid(l) => l,
};
self.cx.add_drop_live_facts_for(local, local_ty, &[location], &locations);
}
}
}
Some(())
}

/// Clear the value of fields that are "per local variable".
Expand Down
10 changes: 1 addition & 9 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,15 +188,7 @@ pub(crate) fn type_check<'mir, 'tcx>(
checker.equate_inputs_and_outputs(body, universal_regions, &normalized_inputs_and_output);
checker.check_signature_annotation(body);

liveness::generate(
&mut checker,
body,
elements,
flow_inits,
move_data,
location_table,
use_polonius,
);
liveness::generate(&mut checker, body, elements, flow_inits, move_data, use_polonius);

translate_outlives_facts(&mut checker);
let opaque_type_values = infcx.take_opaque_types();
Expand Down
Loading