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

Jump to bottom

Treat projections with infer as placeholder during fast reject in new solver #108830

Merged
merged 3 commits into from
Mar 14, 2023
Merged

Treat projections with infer as placeholder during fast reject in new solver #108830

merged 3 commits into from
Mar 14, 2023

Conversation

compiler-errors
Copy link
Member

r? @lcnr

Kind of a shame that we need to change all of the call sites for for_each_relevant_impl, etc. to pass an extra parameter. I guess I could have the "default" fn which calls a configurable fn?

@rustbot 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the core trait solver

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

@compiler-errors compiler-errors force-pushed the new-solver-fast-reject-faster branch 5 times, most recently from ab25527 to 540a9a0 Compare March 7, 2023 05:24
lcnr
lcnr reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

As we share TreatParams with the DeepRejectCtxt, should we instead add TreatProjections separately as another argument to simplify_type and keep the current TreatParams?

compiler/rustc_middle/src/ty/fast_reject.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/trait_def.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/fast_reject.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Mar 8, 2023

I guess I could have the "default" fn which calls a configurable fn?

that would work and is probably a bit cleaner 🤔

@bors
Copy link
Contributor

bors commented Mar 11, 2023

☔ The latest upstream changes (presumably #109019) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors compiler-errors force-pushed the new-solver-fast-reject-faster branch 2 times, most recently from 7661cf3 to dd430d7 Compare March 12, 2023 01:11
lcnr
lcnr reviewed Mar 13, 2023
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

r=me after nits

compiler/rustc_trait_selection/src/solve/trait_goals.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs Outdated Show resolved Hide resolved
tests/ui/traits/new-solver/nested-obligations-with-bound-vars-gat.rs
@@ -0,0 +1,43 @@
// check-pass
// compile-flags: -Ztrait-solver=next
// Issue 96230
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add a fixed-by-trait-solver-next label for issues like this, so that we don't forget to close them when we stabilize the new solver

@compiler-errors compiler-errors force-pushed the new-solver-fast-reject-faster branch from dd430d7 to 84d254e Compare March 13, 2023 16:43
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Mar 13, 2023

📌 Commit 84d254e has been approved by lcnr

It is now in the queue for this repository.

@bors 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 Mar 13, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2023
@matthiaskrgr
Rollup merge of rust-lang#108830 - compiler-errors:new-solver-fast-re…
350df98 
…ject-faster, r=lcnr

Treat projections with infer as placeholder during fast reject in new solver

r? `@lcnr`

Kind of a shame that we need to change all of the call sites for `for_each_relevant_impl`, etc. to pass an extra parameter. I guess I could have the "default" fn which calls a configurable fn?
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 13, 2023
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 13, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2023
@bors
Auto merge of rust-lang#109097 - matthiaskrgr:rollup-6ydc4ri, r=matth…
c54af45 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#108419 (Stabilize `atomic_as_ptr`)
 - rust-lang#108507 (use `as_ptr` to determine the address of atomics)
 - rust-lang#108607 (Don't use fd-lock on Solaris in bootstrap)
 - rust-lang#108830 (Treat projections with infer as placeholder during fast reject in new solver)
 - rust-lang#109055 (create `config::tests::detect_src_and_out` test for bootstrap)
 - rust-lang#109058 (Document BinOp::is_checkable)
 - rust-lang#109081 (simd-wide-sum test: adapt for LLVM 17 codegen change)
 - rust-lang#109083 (Update books)
 - rust-lang#109088 (Gracefully handle `#[target_feature]` on statics)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6cec8cb into rust-lang:master Mar 14, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 14, 2023
@xFrednet xFrednet mentioned this pull request Mar 14, 2023
Closed
@Mark-Simulacrum Mark-Simulacrum added the perf-regression Performance regression. label Mar 14, 2023
@Mark-Simulacrum
Copy link
Member

This caused a perf regression (see results). @lcnr / @compiler-errors do we have a sense of whether this was expected or not? Any thoughts on possible fixes?

@lcnr
Copy link
Contributor

lcnr commented Mar 14, 2023

ahh, that code is really hot so this small change has such a big impact 🤔

we should maybe change simplify_type to be generic over TreatParams and TreatProjections instead 🤔

@compiler-errors
Copy link
Member Author

I can take a look at doing that unless lcnr beats me to it

@nnethercote
Copy link
Contributor

I am surprised by this regression. I thought the fast reject code was very important for bitmaps-3.1.0 and not that important for other things. But the numbers don't lie.

@compiler-errors compiler-errors mentioned this pull request Mar 18, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2023
@bors
Auto merge of rust-lang#109202 - compiler-errors:new-solver-fast-reje…
cf073ec 
…ct-faster-2, r=lcnr

Don't pass `TreatProjections` separately to `fast_reject`

Don't pass `TreatProjections` separately to `fast_reject`, and instead use the original approach of switching on two variants of `TreatParams` (undoes this: rust-lang#108830 (review)).

Fixes the regression introduced in rust-lang#108830 (comment)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Mar 24, 2023
@bors
Auto merge of #109202 - compiler-errors:new-solver-fast-reject-faster…
51324ce 
…-2, r=lcnr

Don't pass `TreatProjections` separately to `fast_reject`

Don't pass `TreatProjections` separately to `fast_reject`, and instead use the original approach of switching on two variants of `TreatParams` (undoes this: rust-lang/rust#108830 (review)).

Fixes the regression introduced in rust-lang/rust#108830 (comment)
@compiler-errors compiler-errors deleted the new-solver-fast-reject-faster branch August 11, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.
6 participants
@compiler-errors @rustbot @lcnr @bors @Mark-Simulacrum @nnethercote