Use new solver in `evaluate_obligation` query (when new solver is enabled) #107103

compiler-errors · 2023-01-20T05:00:20Z

(only when -Ztrait-solver=next, of course)

... Does this make sense? It seems to me like it should be reasonable, but maybe there's some reason why this is a bad idea.

r? @lcnr

Needs a perf run because I guess this solver == TraitSolver::Next check is on a hot path.

rustbot · 2023-01-20T05:00:28Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

compiler-errors · 2023-01-20T05:00:36Z

@bors try @rust-timer queue

bors · 2023-01-20T05:00:46Z

⌛ Trying commit 3e31e7b112ec1faffa1f32f1423185fb70ecc474 with merge 198dee10dce45e624f5a7e09f329deac21a34261...

bors · 2023-01-20T07:42:39Z

☀️ Try build successful - checks-actions
Build commit: 198dee10dce45e624f5a7e09f329deac21a34261 (198dee10dce45e624f5a7e09f329deac21a34261)

rust-timer · 2023-01-20T09:00:24Z

Finished benchmarking commit (198dee10dce45e624f5a7e09f329deac21a34261): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-1.2%	[-1.3%, -1.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-1.3%, -1.1%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

lcnr

I would like to also do the same for evaluate_predicates_recursively 🤔 and then ICE if -Ztrait-solver=next is ever enabled in evaluate_predicate_recursively. That would mean that apart from shallow selection, we never use the old solver when testing the old solver. That will need another perf run though as that code is even hotter.

apart from that 👍

compiler/rustc_traits/src/evaluate_obligation.rs

lcnr · 2023-01-20T16:32:14Z

@rustbot author

compiler/rustc_trait_selection/src/solve/mod.rs

compiler-errors · 2023-01-21T16:49:35Z

I know this isn't the final approach, but let's see how hot these if statements are in these calls are.

@bors try @rust-timer

compiler-errors · 2023-01-21T16:49:41Z

@rust-timer queue

bors · 2023-01-21T16:49:45Z

⌛ Trying commit 3ef7bb2836d152bc0a2a291f18f66b48916ec33b with merge b9518856fe196e67df8bd4040c2562710219eda2...

bors · 2023-01-21T19:14:09Z

☀️ Try build successful - checks-actions
Build commit: b9518856fe196e67df8bd4040c2562710219eda2 (b9518856fe196e67df8bd4040c2562710219eda2)

rust-timer · 2023-01-21T22:00:56Z

Finished benchmarking commit (b9518856fe196e67df8bd4040c2562710219eda2): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	2
Regressions ❌ (secondary)	0.8%	[0.6%, 0.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.7%	[-6.5%, -2.8%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

oli-obk · 2023-01-26T10:49:25Z

@bors rollup=always

lcnr · 2023-01-26T10:56:58Z

@bors r- 😅

lcnr · 2023-01-26T10:59:42Z

going to remove TyCtxtExt::evaluate_goal in #107321 because I want us to always use infcx.evaluate_root_goal instead. Having a solver API which doesn't canonicalize for you causes us to accidentally use a different canonicalization routine which will cause all kinds of issues (and causes us to cache less).

I think we should instead change infcx.evaluate_obligation to branch of -Ztrait-solver=next and call infcx.evaluate_root_obligation there without canonicalizing ourselves.

compiler-errors · 2023-01-26T15:02:22Z

Makes sense

compiler-errors · 2023-01-26T20:09:02Z

@rustbot ready

This could take a quick look, I'm using the same strategy as evaluation_probe to make sure we properly return EvaluatedToOkModulo{Opaques,Regions}.

lcnr · 2023-01-27T06:39:26Z

can you move this to fn InferCtxt::evaluate_obligation? It feels unnecessary to use the query cache considering that we already have a trait solver cache (or well, will have)

r=me on the impl using probe though

compiler-errors · 2023-01-27T07:01:36Z

compiler/rustc_trait_selection/src/solve/mod.rs

@@ -141,11 +147,11 @@ type CanonicalResponse<'tcx> = Canonical<'tcx, Response<'tcx>>;
 /// solver, merge the two responses again.
 pub type QueryResult<'tcx> = Result<CanonicalResponse<'tcx>, NoSolution>;

-pub trait TyCtxtExt<'tcx> {
+pub trait TyCtxtSolveExt<'tcx> {


oops i guess i should revert this too lol

compiler-errors · 2023-01-27T17:54:08Z

compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs

@@ -92,6 +120,9 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> {
        &self,
        obligation: &PredicateObligation<'tcx>,
    ) -> EvaluationResult {
+        // Run canonical query. If overflow occurs, rerun from scratch but this time


This comment is a leftover from 2fd5516, and should have been moved to this function instead of left in evaluate_obligation, where this behavior notable does not occur.

compiler-errors · 2023-01-27T18:33:08Z

@bors r=lcnr rollup=maybe

bors · 2023-01-27T18:33:10Z

📌 Commit 5bfd90e has been approved by lcnr

It is now in the queue for this repository.

…e_obligation, r=lcnr Use new solver in `evaluate_obligation` query (when new solver is enabled) (only when `-Ztrait-solver=next`, of course) ... Does this make sense? It seems to me like it should be reasonable, but maybe there's some reason why this is a bad idea. r? `@lcnr` Needs a perf run because I guess this `solver == TraitSolver::Next` check is on a hot path.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#107022 (Implement `SpecOptionPartialEq` for `cmp::Ordering`) - rust-lang#107100 (Use proper `InferCtxt` when probing for associated types in astconv) - rust-lang#107103 (Use new solver in `evaluate_obligation` query (when new solver is enabled)) - rust-lang#107190 (Recover from more const arguments that are not wrapped in curly braces) - rust-lang#107306 (Correct suggestions for closure arguments that need a borrow) - rust-lang#107339 (internally change regions to be covariant) - rust-lang#107344 (Minor tweaks in the new solver) - rust-lang#107373 (Don't merge vtables when full debuginfo is enabled.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned lcnr Jan 20, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Jan 20, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2023

compiler-errors changed the title ~~Use new solver in evaluate_obligation query~~ Use new solver in evaluate_obligation query (when new solver is enabled) Jan 20, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2023

lcnr reviewed Jan 20, 2023

View reviewed changes

compiler/rustc_traits/src/evaluate_obligation.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2023

compiler-errors force-pushed the new-solver-evaluate_obligation branch from 3e31e7b to 3ef7bb2 Compare January 20, 2023 20:28

compiler-errors commented Jan 20, 2023

View reviewed changes

compiler/rustc_trait_selection/src/solve/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 21, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 26, 2023

compiler-errors force-pushed the new-solver-evaluate_obligation branch from 9dc2e24 to b78f8de Compare January 26, 2023 20:08

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 26, 2023

compiler-errors commented Jan 27, 2023

View reviewed changes

lcnr mentioned this pull request Jan 27, 2023

Tracking Issue for -Znext-solver #107374

Open

11 tasks

Use now solver in evaluate_obligation

5bfd90e

compiler-errors force-pushed the new-solver-evaluate_obligation branch from b78f8de to 5bfd90e Compare January 27, 2023 17:53

compiler-errors commented Jan 27, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2023

matthiaskrgr mentioned this pull request Jan 27, 2023

Rollup of 9 pull requests #107390

Closed

matthiaskrgr mentioned this pull request Jan 28, 2023

Rollup of 8 pull requests #107400

Merged

bors merged commit 3b6593a into rust-lang:master Jan 28, 2023

rustbot added this to the 1.69.0 milestone Jan 28, 2023

compiler-errors mentioned this pull request Mar 30, 2023

Clone region var origins instead of taking them in borrowck #109753

Merged

compiler-errors deleted the new-solver-evaluate_obligation branch August 11, 2023 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new solver in `evaluate_obligation` query (when new solver is enabled) #107103

Use new solver in `evaluate_obligation` query (when new solver is enabled) #107103

compiler-errors commented Jan 20, 2023 •

edited

Loading

rustbot commented Jan 20, 2023

compiler-errors commented Jan 20, 2023

This comment has been minimized.

bors commented Jan 20, 2023

bors commented Jan 20, 2023

This comment has been minimized.

rust-timer commented Jan 20, 2023

lcnr left a comment

lcnr commented Jan 20, 2023

compiler-errors commented Jan 21, 2023

compiler-errors commented Jan 21, 2023

This comment has been minimized.

bors commented Jan 21, 2023

bors commented Jan 21, 2023

This comment has been minimized.

rust-timer commented Jan 21, 2023

oli-obk commented Jan 26, 2023

lcnr commented Jan 26, 2023

lcnr commented Jan 26, 2023

compiler-errors commented Jan 26, 2023

compiler-errors commented Jan 26, 2023

lcnr commented Jan 27, 2023

compiler-errors Jan 27, 2023

compiler-errors Jan 27, 2023

compiler-errors commented Jan 27, 2023

bors commented Jan 27, 2023

Use new solver in evaluate_obligation query (when new solver is enabled) #107103

Use new solver in evaluate_obligation query (when new solver is enabled) #107103

Conversation

compiler-errors commented Jan 20, 2023 • edited Loading

rustbot commented Jan 20, 2023

compiler-errors commented Jan 20, 2023

This comment has been minimized.

bors commented Jan 20, 2023

bors commented Jan 20, 2023

This comment has been minimized.

rust-timer commented Jan 20, 2023

Overall result: no relevant changes - no action needed

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Jan 20, 2023

compiler-errors commented Jan 21, 2023

compiler-errors commented Jan 21, 2023

This comment has been minimized.

bors commented Jan 21, 2023

bors commented Jan 21, 2023

This comment has been minimized.

rust-timer commented Jan 21, 2023

Overall result: ❌ regressions - ACTION NEEDED

oli-obk commented Jan 26, 2023

lcnr commented Jan 26, 2023

lcnr commented Jan 26, 2023

compiler-errors commented Jan 26, 2023

compiler-errors commented Jan 26, 2023

lcnr commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Jan 27, 2023

bors commented Jan 27, 2023

Use new solver in `evaluate_obligation` query (when new solver is enabled) #107103

Use new solver in `evaluate_obligation` query (when new solver is enabled) #107103

compiler-errors commented Jan 20, 2023 •

edited

Loading