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

cleanup bound variable handling #97648

Merged
merged 8 commits into from
Jun 11, 2022
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
12 changes: 5 additions & 7 deletions compiler/rustc_borrowck/src/type_check/input_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,11 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
// Replace the bound items in the fn sig with fresh
// variables, so that they represent the view from
// "inside" the closure.
self.infcx
.replace_bound_vars_with_fresh_vars(
body.span,
LateBoundRegionConversionTime::FnCall,
poly_sig,
)
.0
self.infcx.replace_bound_vars_with_fresh_vars(
body.span,
LateBoundRegionConversionTime::FnCall,
poly_sig,
)
},
);
}
Expand Down
14 changes: 8 additions & 6 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_infer::infer::outlives::env::RegionBoundPairs;
use rustc_infer::infer::region_constraints::RegionConstraintData;
use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use rustc_infer::infer::{
InferCtxt, InferOk, LateBoundRegionConversionTime, NllRegionVariableOrigin,
InferCtxt, InferOk, LateBoundRegion, LateBoundRegionConversionTime, NllRegionVariableOrigin,
};
use rustc_middle::mir::tcx::PlaceTy;
use rustc_middle::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor};
Expand Down Expand Up @@ -1436,11 +1436,13 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
return;
}
};
let (sig, map) = self.infcx.replace_bound_vars_with_fresh_vars(
term.source_info.span,
LateBoundRegionConversionTime::FnCall,
sig,
);
let (sig, map) = tcx.replace_late_bound_regions(sig, |br| {
self.infcx.next_region_var(LateBoundRegion(
term.source_info.span,
br.kind,
LateBoundRegionConversionTime::FnCall,
))
});
debug!(?sig);
let sig = self.normalize(sig, term_location);
self.check_call_dest(body, term, &sig, *destination, target, term_location);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_infer/src/infer/canonical/substitute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,6 @@ where
c => bug!("{:?} is a const but value is {:?}", bound_ct, c),
};

tcx.replace_escaping_bound_vars(value, fld_r, fld_t, fld_c)
tcx.replace_escaping_bound_vars_uncached(value, fld_r, fld_t, fld_c)
}
}
4 changes: 2 additions & 2 deletions compiler/rustc_infer/src/infer/equate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,12 @@ impl<'tcx> TypeRelation<'tcx> for Equate<'_, '_, 'tcx> {
{
if a.skip_binder().has_escaping_bound_vars() || b.skip_binder().has_escaping_bound_vars() {
self.fields.higher_ranked_sub(a, b, self.a_is_expected)?;
self.fields.higher_ranked_sub(b, a, self.a_is_expected)
self.fields.higher_ranked_sub(b, a, self.a_is_expected)?;
} else {
// Fast path for the common case.
self.relate(a.skip_binder(), b.skip_binder())?;
Ok(a)
}
Ok(a)
}
}

Expand Down
94 changes: 43 additions & 51 deletions compiler/rustc_infer/src/infer/higher_ranked/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,78 +3,85 @@

use super::combine::CombineFields;
use super::{HigherRankedType, InferCtxt};

use crate::infer::CombinedSnapshot;
use rustc_middle::ty::relate::{Relate, RelateResult, TypeRelation};
use rustc_middle::ty::{self, Binder, TypeFoldable};

impl<'a, 'tcx> CombineFields<'a, 'tcx> {
/// Checks whether `for<..> sub <: for<..> sup` holds.
///
/// For this to hold, **all** instantiations of the super type
/// have to be a super type of **at least one** instantiation of
/// the subtype.
///
/// This is implemented by first entering a new universe.
/// We then replace all bound variables in `sup` with placeholders,
/// and all bound variables in `sup` with inference vars.
/// We can then just relate the two resulting types as normal.
///
/// Note: this is a subtle algorithm. For a full explanation, please see
/// the [rustc dev guide][rd]
///
/// [rd]: https://rustc-dev-guide.rust-lang.org/borrow_check/region_inference/placeholders_and_universes.html
#[instrument(skip(self), level = "debug")]
pub fn higher_ranked_sub<T>(
&mut self,
a: Binder<'tcx, T>,
b: Binder<'tcx, T>,
a_is_expected: bool,
) -> RelateResult<'tcx, Binder<'tcx, T>>
sub: Binder<'tcx, T>,
sup: Binder<'tcx, T>,
sub_is_expected: bool,
) -> RelateResult<'tcx, ()>
where
T: Relate<'tcx>,
{
// Rather than checking the subtype relationship between `a` and `b`
// as-is, we need to do some extra work here in order to make sure
// that function subtyping works correctly with respect to regions
//
// Note: this is a subtle algorithm. For a full explanation, please see
// the rustc dev guide:
// <https://rustc-dev-guide.rust-lang.org/borrow_check/region_inference/placeholders_and_universes.html>

let span = self.trace.cause.span;

self.infcx.commit_if_ok(|_| {
// First, we instantiate each bound region in the supertype with a
// fresh placeholder region.
let b_prime = self.infcx.replace_bound_vars_with_placeholders(b);
// fresh placeholder region. Note that this automatically creates
// a new universe if needed.
let sup_prime = self.infcx.replace_bound_vars_with_placeholders(sup);

// Next, we instantiate each bound region in the subtype
// with a fresh region variable. These region variables --
// but no other pre-existing region variables -- can name
// the placeholders.
let (a_prime, _) =
self.infcx.replace_bound_vars_with_fresh_vars(span, HigherRankedType, a);
let sub_prime =
self.infcx.replace_bound_vars_with_fresh_vars(span, HigherRankedType, sub);

debug!("a_prime={:?}", a_prime);
debug!("b_prime={:?}", b_prime);
debug!("a_prime={:?}", sub_prime);
debug!("b_prime={:?}", sup_prime);

// Compare types now that bound regions have been replaced.
let result = self.sub(a_is_expected).relate(a_prime, b_prime)?;

debug!("higher_ranked_sub: OK result={:?}", result);
let result = self.sub(sub_is_expected).relate(sub_prime, sup_prime)?;

// We related `a_prime` and `b_prime`, which just had any bound vars
// replaced with placeholders or infer vars, respectively. Relating
// them should not introduce new bound vars.
Ok(ty::Binder::dummy(result))
debug!("higher_ranked_sub: OK result={result:?}");
// NOTE: returning the result here would be dangerous as it contains
// placeholders which **must not** be named afterwards.
Ok(())
})
}
}

impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
/// Replaces all bound variables (lifetimes, types, and constants) bound by
/// `binder` with placeholder variables.
/// `binder` with placeholder variables in a new universe. This means that the
/// new placeholders can only be named by inference variables created after
/// this method has been called.
///
/// This is the first step of checking subtyping when higher-ranked things are involved.
/// For more details visit the relevant sections of the [rustc dev guide].
///
/// [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/traits/hrtb.html
#[instrument(level = "debug", skip(self))]
pub fn replace_bound_vars_with_placeholders<T>(&self, binder: ty::Binder<'tcx, T>) -> T
where
T: TypeFoldable<'tcx>,
T: TypeFoldable<'tcx> + Copy,
{
// Figure out what the next universe will be, but don't actually create
// it until after we've done the substitution (in particular there may
// be no bound variables). This is a performance optimization, since the
// leak check for example can be skipped if no new universes are created
// (i.e., if there are no placeholders).
let next_universe = self.universe().next_universe();
if let Some(inner) = binder.no_bound_vars() {
return inner;
}

let next_universe = self.create_next_universe();

let fld_r = |br: ty::BoundRegion| {
self.tcx.mk_region(ty::RePlaceholder(ty::PlaceholderRegion {
Expand All @@ -100,23 +107,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
})
};

let (result, map) = self.tcx.replace_bound_vars(binder, fld_r, fld_t, fld_c);

// If there were higher-ranked regions to replace, then actually create
// the next universe (this avoids needlessly creating universes).
if !map.is_empty() {
let n_u = self.create_next_universe();
assert_eq!(n_u, next_universe);
}

debug!(
"replace_bound_vars_with_placeholders(\
next_universe={:?}, \
result={:?}, \
map={:?})",
next_universe, result, map,
);

let result = self.tcx.replace_bound_vars_uncached(binder, fld_r, fld_t, fld_c);
debug!(?next_universe, ?result);
result
}

Expand Down
44 changes: 29 additions & 15 deletions compiler/rustc_infer/src/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ use rustc_span::symbol::Symbol;
use rustc_span::Span;

use std::cell::{Cell, Ref, RefCell};
use std::collections::BTreeMap;
use std::fmt;

use self::combine::CombineFields;
Expand Down Expand Up @@ -1524,25 +1523,40 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
span: Span,
lbrct: LateBoundRegionConversionTime,
value: ty::Binder<'tcx, T>,
) -> (T, BTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
) -> T
where
T: TypeFoldable<'tcx>,
T: TypeFoldable<'tcx> + Copy,
{
let fld_r =
|br: ty::BoundRegion| self.next_region_var(LateBoundRegion(span, br.kind, lbrct));
let fld_t = |_| {
self.next_ty_var(TypeVariableOrigin {
kind: TypeVariableOriginKind::MiscVariable,
span,
if let Some(inner) = value.no_bound_vars() {
return inner;
}

let mut region_map = FxHashMap::default();
Copy link
Contributor Author

@lcnr lcnr Jun 10, 2022

Choose a reason for hiding this comment

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

it looks like the perf regression is caused by this code being pretty hot and us always creating 3 separate storages here and then having to drop them.

Went from BTreeMap -> SsoHashMap -> FxHashMap to figure out if these help. The more complex Drop impl (and greater size) of SsoHashMap made things worse. So hopefully the comparatively simple HashMap ends up being an improvement.

If not I think we should commit to tracking bound vars in binders and modify this to eagerly create a Vec of inference vars using value.bound_vars() and then using that. Doing so should allow us to get some perf improvements in other areas as well.

let fld_r = |br: ty::BoundRegion| {
*region_map
.entry(br)
.or_insert_with(|| self.next_region_var(LateBoundRegion(span, br.kind, lbrct)))
};

let mut ty_map = FxHashMap::default();
let fld_t = |bt: ty::BoundTy| {
*ty_map.entry(bt).or_insert_with(|| {
self.next_ty_var(TypeVariableOrigin {
kind: TypeVariableOriginKind::MiscVariable,
span,
})
})
};
let fld_c = |_, ty| {
self.next_const_var(
ty,
ConstVariableOrigin { kind: ConstVariableOriginKind::MiscVariable, span },
)
let mut ct_map = FxHashMap::default();
let fld_c = |bc: ty::BoundVar, ty| {
*ct_map.entry(bc).or_insert_with(|| {
self.next_const_var(
ty,
ConstVariableOrigin { kind: ConstVariableOriginKind::MiscVariable, span },
)
})
};
self.tcx.replace_bound_vars(value, fld_r, fld_t, fld_c)
self.tcx.replace_bound_vars_uncached(value, fld_r, fld_t, fld_c)
}

/// See the [`region_constraints::RegionConstraintCollector::verify_generic_bound`] method.
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_infer/src/infer/sub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,8 @@ impl<'tcx> TypeRelation<'tcx> for Sub<'_, '_, 'tcx> {
where
T: Relate<'tcx>,
{
self.fields.higher_ranked_sub(a, b, self.a_is_expected)
self.fields.higher_ranked_sub(a, b, self.a_is_expected)?;
Ok(a)
}
}

Expand Down
Loading