Skip to content

Commit

Permalink
Rollup merge of rust-lang#131491 - lcnr:nalgebra-perrrrf, r=compiler-…
Browse files Browse the repository at this point in the history
…errors

impossible obligations fast path

fixes the remaining performance regression in nalgebra for rust-lang#130654

r? `@compiler-errors`

Fixes rust-lang#124894
  • Loading branch information
matthiaskrgr authored Oct 10, 2024
2 parents 7eb5621 + d6fd45c commit 1c62cff
Show file tree
Hide file tree
Showing 8 changed files with 90 additions and 45 deletions.
72 changes: 54 additions & 18 deletions compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ use crate::solve::inspect::{self, ProofTreeBuilder};
use crate::solve::search_graph::SearchGraph;
use crate::solve::{
CanonicalInput, CanonicalResponse, Certainty, FIXPOINT_STEP_LIMIT, Goal, GoalEvaluationKind,
GoalSource, NestedNormalizationGoals, NoSolution, PredefinedOpaquesData, QueryResult,
SolverMode,
GoalSource, HasChanged, NestedNormalizationGoals, NoSolution, PredefinedOpaquesData,
QueryResult, SolverMode,
};

pub(super) mod canonical;
Expand Down Expand Up @@ -126,11 +126,31 @@ pub enum GenerateProofTree {
}

pub trait SolverDelegateEvalExt: SolverDelegate {
/// Evaluates a goal from **outside** of the trait solver.
///
/// Using this while inside of the solver is wrong as it uses a new
/// search graph which would break cycle detection.
fn evaluate_root_goal(
&self,
goal: Goal<Self::Interner, <Self::Interner as Interner>::Predicate>,
generate_proof_tree: GenerateProofTree,
) -> (Result<(bool, Certainty), NoSolution>, Option<inspect::GoalEvaluation<Self::Interner>>);
) -> (
Result<(HasChanged, Certainty), NoSolution>,
Option<inspect::GoalEvaluation<Self::Interner>>,
);

/// Check whether evaluating `goal` with a depth of `root_depth` may
/// succeed. This only returns `false` if the goal is guaranteed to
/// not hold. In case evaluation overflows and fails with ambiguity this
/// returns `true`.
///
/// This is only intended to be used as a performance optimization
/// in coherence checking.
fn root_goal_may_hold_with_depth(
&self,
root_depth: usize,
goal: Goal<Self::Interner, <Self::Interner as Interner>::Predicate>,
) -> bool;

// FIXME: This is only exposed because we need to use it in `analyse.rs`
// which is not yet uplifted. Once that's done, we should remove this.
Expand All @@ -139,7 +159,7 @@ pub trait SolverDelegateEvalExt: SolverDelegate {
goal: Goal<Self::Interner, <Self::Interner as Interner>::Predicate>,
generate_proof_tree: GenerateProofTree,
) -> (
Result<(NestedNormalizationGoals<Self::Interner>, bool, Certainty), NoSolution>,
Result<(NestedNormalizationGoals<Self::Interner>, HasChanged, Certainty), NoSolution>,
Option<inspect::GoalEvaluation<Self::Interner>>,
);
}
Expand All @@ -149,31 +169,41 @@ where
D: SolverDelegate<Interner = I>,
I: Interner,
{
/// Evaluates a goal from **outside** of the trait solver.
///
/// Using this while inside of the solver is wrong as it uses a new
/// search graph which would break cycle detection.
#[instrument(level = "debug", skip(self))]
fn evaluate_root_goal(
&self,
goal: Goal<I, I::Predicate>,
generate_proof_tree: GenerateProofTree,
) -> (Result<(bool, Certainty), NoSolution>, Option<inspect::GoalEvaluation<I>>) {
EvalCtxt::enter_root(self, generate_proof_tree, |ecx| {
) -> (Result<(HasChanged, Certainty), NoSolution>, Option<inspect::GoalEvaluation<I>>) {
EvalCtxt::enter_root(self, self.cx().recursion_limit(), generate_proof_tree, |ecx| {
ecx.evaluate_goal(GoalEvaluationKind::Root, GoalSource::Misc, goal)
})
}

fn root_goal_may_hold_with_depth(
&self,
root_depth: usize,
goal: Goal<Self::Interner, <Self::Interner as Interner>::Predicate>,
) -> bool {
self.probe(|| {
EvalCtxt::enter_root(self, root_depth, GenerateProofTree::No, |ecx| {
ecx.evaluate_goal(GoalEvaluationKind::Root, GoalSource::Misc, goal)
})
.0
})
.is_ok()
}

#[instrument(level = "debug", skip(self))]
fn evaluate_root_goal_raw(
&self,
goal: Goal<I, I::Predicate>,
generate_proof_tree: GenerateProofTree,
) -> (
Result<(NestedNormalizationGoals<I>, bool, Certainty), NoSolution>,
Result<(NestedNormalizationGoals<I>, HasChanged, Certainty), NoSolution>,
Option<inspect::GoalEvaluation<I>>,
) {
EvalCtxt::enter_root(self, generate_proof_tree, |ecx| {
EvalCtxt::enter_root(self, self.cx().recursion_limit(), generate_proof_tree, |ecx| {
ecx.evaluate_goal_raw(GoalEvaluationKind::Root, GoalSource::Misc, goal)
})
}
Expand All @@ -197,10 +227,11 @@ where
/// over using this manually (such as [`SolverDelegateEvalExt::evaluate_root_goal`]).
pub(super) fn enter_root<R>(
delegate: &D,
root_depth: usize,
generate_proof_tree: GenerateProofTree,
f: impl FnOnce(&mut EvalCtxt<'_, D>) -> R,
) -> (R, Option<inspect::GoalEvaluation<I>>) {
let mut search_graph = SearchGraph::new(delegate.solver_mode());
let mut search_graph = SearchGraph::new(delegate.solver_mode(), root_depth);

let mut ecx = EvalCtxt {
delegate,
Expand Down Expand Up @@ -339,7 +370,7 @@ where
goal_evaluation_kind: GoalEvaluationKind,
source: GoalSource,
goal: Goal<I, I::Predicate>,
) -> Result<(bool, Certainty), NoSolution> {
) -> Result<(HasChanged, Certainty), NoSolution> {
let (normalization_nested_goals, has_changed, certainty) =
self.evaluate_goal_raw(goal_evaluation_kind, source, goal)?;
assert!(normalization_nested_goals.is_empty());
Expand All @@ -360,7 +391,7 @@ where
goal_evaluation_kind: GoalEvaluationKind,
_source: GoalSource,
goal: Goal<I, I::Predicate>,
) -> Result<(NestedNormalizationGoals<I>, bool, Certainty), NoSolution> {
) -> Result<(NestedNormalizationGoals<I>, HasChanged, Certainty), NoSolution> {
let (orig_values, canonical_goal) = self.canonicalize_goal(goal);
let mut goal_evaluation =
self.inspect.new_goal_evaluation(goal, &orig_values, goal_evaluation_kind);
Expand All @@ -378,8 +409,13 @@ where
Ok(response) => response,
};

let has_changed = !response.value.var_values.is_identity_modulo_regions()
|| !response.value.external_constraints.opaque_types.is_empty();
let has_changed = if !response.value.var_values.is_identity_modulo_regions()
|| !response.value.external_constraints.opaque_types.is_empty()
{
HasChanged::Yes
} else {
HasChanged::No
};

let (normalization_nested_goals, certainty) =
self.instantiate_and_apply_query_response(goal.param_env, orig_values, response);
Expand Down Expand Up @@ -552,7 +588,7 @@ where
for (source, goal) in goals.goals {
let (has_changed, certainty) =
self.evaluate_goal(GoalEvaluationKind::Nested, source, goal)?;
if has_changed {
if has_changed == HasChanged::Yes {
unchanged_certainty = None;
}

Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_next_trait_solver/src/solve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ enum GoalEvaluationKind {
Nested,
}

/// Whether evaluating this goal ended up changing the
/// inference state.
#[derive(PartialEq, Eq, Debug, Hash, Clone, Copy)]
pub enum HasChanged {
Yes,
No,
}

// FIXME(trait-system-refactor-initiative#117): we don't detect whether a response
// ended up pulling down any universes.
fn has_no_inference_or_external_constraints<I: Interner>(
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_next_trait_solver/src/solve/search_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,6 @@ where
}

const DIVIDE_AVAILABLE_DEPTH_ON_OVERFLOW: usize = 4;
fn recursion_limit(cx: I) -> usize {
cx.recursion_limit()
}

fn initial_provisional_result(
cx: I,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_trait_selection/src/solve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod inspect;
mod normalize;
mod select;

pub(crate) use delegate::SolverDelegate;
pub use fulfill::{FulfillmentCtxt, NextSolverError};
pub(crate) use normalize::deeply_normalize_for_diagnostics;
pub use normalize::{deeply_normalize, deeply_normalize_with_skipped_universes};
Expand Down
15 changes: 8 additions & 7 deletions compiler/rustc_trait_selection/src/solve/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_infer::traits::{
use rustc_middle::bug;
use rustc_middle::ty::error::{ExpectedFound, TypeError};
use rustc_middle::ty::{self, TyCtxt};
use rustc_next_trait_solver::solve::{GenerateProofTree, SolverDelegateEvalExt as _};
use rustc_next_trait_solver::solve::{GenerateProofTree, HasChanged, SolverDelegateEvalExt as _};
use tracing::instrument;

use super::Certainty;
Expand Down Expand Up @@ -86,10 +86,7 @@ impl<'tcx> ObligationStorage<'tcx> {
let result = <&SolverDelegate<'tcx>>::from(infcx)
.evaluate_root_goal(goal, GenerateProofTree::No)
.0;
match result {
Ok((has_changed, _)) => has_changed,
_ => false,
}
matches!(result, Ok((HasChanged::Yes, _)))
}));
})
}
Expand All @@ -113,7 +110,7 @@ impl<'tcx, E: 'tcx> FulfillmentCtxt<'tcx, E> {
&self,
infcx: &InferCtxt<'tcx>,
obligation: &PredicateObligation<'tcx>,
result: &Result<(bool, Certainty), NoSolution>,
result: &Result<(HasChanged, Certainty), NoSolution>,
) {
if let Some(inspector) = infcx.obligation_inspector.get() {
let result = match result {
Expand Down Expand Up @@ -181,7 +178,11 @@ where
continue;
}
};
has_changed |= changed;

if changed == HasChanged::Yes {
has_changed = true;
}

match certainty {
Certainty::Yes => {}
Certainty::Maybe(_) => self.obligations.register(obligation),
Expand Down
13 changes: 12 additions & 1 deletion compiler/rustc_trait_selection/src/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use rustc_middle::ty::fast_reject::DeepRejectCtxt;
use rustc_middle::ty::visit::{TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor};
use rustc_middle::ty::{self, Ty, TyCtxt};
pub use rustc_next_trait_solver::coherence::*;
use rustc_next_trait_solver::solve::SolverDelegateEvalExt;
use rustc_span::symbol::sym;
use rustc_span::{DUMMY_SP, Span};
use tracing::{debug, instrument, warn};
Expand All @@ -28,7 +29,7 @@ use crate::error_reporting::traits::suggest_new_overflow_limit;
use crate::infer::InferOk;
use crate::infer::outlives::env::OutlivesEnvironment;
use crate::solve::inspect::{InspectGoal, ProofTreeInferCtxtExt, ProofTreeVisitor};
use crate::solve::{deeply_normalize_for_diagnostics, inspect};
use crate::solve::{SolverDelegate, deeply_normalize_for_diagnostics, inspect};
use crate::traits::query::evaluate_obligation::InferCtxtExt;
use crate::traits::select::IntercrateAmbiguityCause;
use crate::traits::{
Expand Down Expand Up @@ -333,6 +334,16 @@ fn impl_intersection_has_impossible_obligation<'a, 'cx, 'tcx>(
let infcx = selcx.infcx;

if infcx.next_trait_solver() {
// A fast path optimization, try evaluating all goals with
// a very low recursion depth and bail if any of them don't
// hold.
if !obligations.iter().all(|o| {
<&SolverDelegate<'tcx>>::from(infcx)
.root_goal_may_hold_with_depth(8, Goal::new(infcx.tcx, o.param_env, o.predicate))
}) {
return IntersectionHasImpossibleObligations::Yes;
}

let ocx = ObligationCtxt::new_with_diagnostics(infcx);
ocx.register_obligations(obligations.iter().cloned());
let errors_and_ambiguities = ocx.select_all_or_error();
Expand Down
12 changes: 7 additions & 5 deletions compiler/rustc_type_ir/src/search_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ pub trait Delegate {
fn inspect_is_noop(inspect: &mut Self::ProofTreeBuilder) -> bool;

const DIVIDE_AVAILABLE_DEPTH_ON_OVERFLOW: usize;
fn recursion_limit(cx: Self::Cx) -> usize;

fn initial_provisional_result(
cx: Self::Cx,
Expand Down Expand Up @@ -156,7 +155,7 @@ impl AvailableDepth {
/// the remaining depth of all nested goals to prevent hangs
/// in case there is exponential blowup.
fn allowed_depth_for_nested<D: Delegate>(
cx: D::Cx,
root_depth: AvailableDepth,
stack: &IndexVec<StackDepth, StackEntry<D::Cx>>,
) -> Option<AvailableDepth> {
if let Some(last) = stack.raw.last() {
Expand All @@ -170,7 +169,7 @@ impl AvailableDepth {
AvailableDepth(last.available_depth.0 - 1)
})
} else {
Some(AvailableDepth(D::recursion_limit(cx)))
Some(root_depth)
}
}

Expand Down Expand Up @@ -360,6 +359,7 @@ struct ProvisionalCacheEntry<X: Cx> {

pub struct SearchGraph<D: Delegate<Cx = X>, X: Cx = <D as Delegate>::Cx> {
mode: SolverMode,
root_depth: AvailableDepth,
/// The stack of goals currently being computed.
///
/// An element is *deeper* in the stack if its index is *lower*.
Expand All @@ -374,9 +374,10 @@ pub struct SearchGraph<D: Delegate<Cx = X>, X: Cx = <D as Delegate>::Cx> {
}

impl<D: Delegate<Cx = X>, X: Cx> SearchGraph<D> {
pub fn new(mode: SolverMode) -> SearchGraph<D> {
pub fn new(mode: SolverMode, root_depth: usize) -> SearchGraph<D> {
Self {
mode,
root_depth: AvailableDepth(root_depth),
stack: Default::default(),
provisional_cache: Default::default(),
_marker: PhantomData,
Expand Down Expand Up @@ -460,7 +461,8 @@ impl<D: Delegate<Cx = X>, X: Cx> SearchGraph<D> {
inspect: &mut D::ProofTreeBuilder,
mut evaluate_goal: impl FnMut(&mut Self, &mut D::ProofTreeBuilder) -> X::Result,
) -> X::Result {
let Some(available_depth) = AvailableDepth::allowed_depth_for_nested::<D>(cx, &self.stack)
let Some(available_depth) =
AvailableDepth::allowed_depth_for_nested::<D>(self.root_depth, &self.stack)
else {
return self.handle_overflow(cx, input, inspect);
};
Expand Down
11 changes: 0 additions & 11 deletions tests/crashes/124894.rs

This file was deleted.

0 comments on commit 1c62cff

Please sign in to comment.