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

Cleanup: Eliminate ConstnessAnd #91354

Merged
merged 18 commits into from
Dec 2, 2021
Merged

Cleanup: Eliminate ConstnessAnd #91354

merged 18 commits into from
Dec 2, 2021

Conversation

fee1-dead
Copy link
Member

This is almost a behaviour-free change and purely a refactoring. "almost" because we appear to be using the wrong ParamEnv somewhere already, and this is now exposed by failing a test using the unstable ~const feature.

We most definitely need to review all without_const and at some point should probably get rid of many of them by using TraitPredicate instead of TraitRef.

This is a continuation of #90274.

r? @oli-obk

cc @spastorino @ecstatic-morse

oli-obk and others added 14 commits November 29, 2021 21:19
@oli-obk @fee1-dead
Prepare for more ParamEnv flags
22eeff7
@oli-obk @fee1-dead
Add constness to ParamEnv
721ffd1 
This now causes a lot of queries to be executed twice, as reveal_all forces NotConst
@oli-obk @fee1-dead
Static items are also const
1761d88
@oli-obk @fee1-dead
Re-use constness_for_typeck instead of rolling it ourselves
e37947f
@oli-obk @fee1-dead
Fixup add const to param env
19f2101
@oli-obk @fee1-dead
Use the constness from the param env instead of having a separate dim…
d51068c 
…ension for it

This breaks a ~const test that will be fixed in a follow up commit of this PR
@oli-obk @fee1-dead
Fixup
40f39e6
@oli-obk @fee1-dead
Prefer TraitPredicate over ConstnessAnd<TraitRef>
b16c811
@oli-obk @fee1-dead
Replace ConstnessAnd<TraitRef> with TraitPredicate which conveys …
d161cc2 
…the same information
@oli-obk @fee1-dead
Avoid storing the ImplPolarity and Constness next to a TraitRef and u…
a848c4b 
…se TraitPredicate instead
@oli-obk @fee1-dead
Completely remove ConstnessAnd
a9a79f6
@oli-obk @fee1-dead
Always use const param envs for const eval.
f394bb5 
Nothing else makes sense, and there is no "danger" in doing so, as it only does something if there are const bounds, which are unstable. This used to happen implicitly via the inferctxt before, which was much more fragile.
@fee1-dead
Fix tools
fdf5322
@fee1-dead
Reformat everything
8710a2e
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2021
@fee1-dead
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 29, 2021
@bors
Copy link
Contributor

bors commented Nov 29, 2021

⌛ Trying commit 8710a2e with merge 3bde14d5d8c5c093944947c2dfb90ea047842a0b...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2021

r? @spastorino

Can't review a PR made up of mostly my own commits

@rust-highfive rust-highfive assigned spastorino and unassigned oli-obk Nov 29, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2021

Tho... essentially @fee1-dead can review this, you only rebased it and fixed some tools. I guess r=oli-obk,fee1-dead if you are alright with my commits

@fee1-dead
Copy link
Member Author

I don't think I can do that, as this PR requires some work (fixing the assoc type ui test), also improving caching of queries that do not care about constness if the performance had gotten worse.

The ui test fix is quite simple. constness_for_typeck should return Constness::Const when the item is ImplItem::Fn | ImplItem::Const in a const impl. I had to remove the function from rustc_hir as the crate doesn't know what TyCtxt is and therefore cannot query the parent of a node easily.

@fee1-dead fee1-dead marked this pull request as draft November 29, 2021 14:41
@fee1-dead
Copy link
Member Author

@bors r=spastorino

@bors
Copy link
Contributor

bors commented Dec 2, 2021

📌 Commit 5ebc99e has been approved by spastorino

@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 Dec 2, 2021
@bors
Copy link
Contributor

bors commented Dec 2, 2021

⌛ Testing commit 5ebc99e with merge 18bb8c6...

@spastorino
Copy link
Member

I'm not sure how perf labelling works really but seems like we have perf-regression left from the first perf run, but the second shows mixed results.
Should we manually remove perf-regression label? if that's the case maybe it's nice if the perf tooling automatically does that in the second run.

cc @rust-lang/wg-compiler-performance

@fee1-dead
Copy link
Member Author

rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

perfbot always marks PRs with regressions as regressions. It automatically removes the label when there are no regressions, which is not the case for this PR.

@bors
Copy link
Contributor

bors commented Dec 2, 2021

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing 18bb8c6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 2, 2021
@bors bors merged commit 18bb8c6 into rust-lang:master Dec 2, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 2, 2021
This was referenced Dec 2, 2021
Merged
Closed
Merged
Merged
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (18bb8c6): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.6% on full builds of deeply-nested)
  • Moderate regression in instruction counts (up to 1.1% on incr-patched: b9b3e592dd cherry picked builds of style-servo)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@oli-obk
Copy link
Contributor

oli-obk commented Dec 2, 2021

I have an idea that could address the remaining regressions: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/ParamEnv.20and.20constness/near/263477812

@fee1-dead fee1-dead deleted the const_env branch December 3, 2021 10:33
@sdroege sdroege mentioned this pull request Dec 3, 2021
Closed
@sdroege
Copy link
Contributor

sdroege commented Dec 3, 2021

This seems to cause a regression, see #91489

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2021

Oof. That's quite odd. Let's revert this PR and go with a smaller subset of the changes first?

spastorino added a commit to spastorino/rust that referenced this pull request Dec 3, 2021
@spastorino
Revert "Auto merge of rust-lang#91354 - fee1-dead:const_env, r=spasto…
85b723c 
…rino"

This reverts commit 18bb8c6, reversing
changes made to d9baa36.
@spastorino spastorino mentioned this pull request Dec 3, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2021
@bors
Auto merge of rust-lang#91491 - spastorino:revert-91354, r=oli-obk
ff2439b 
Revert "Auto merge of rust-lang#91354 - fee1-dead:const_env, r=spastorino"

This reverts commit 18bb8c6, reversing
changes made to d9baa36.

Reverts rust-lang#91354 in order to address rust-lang#91489. We would need to place this changes in a more granular way and would also be nice to address the small perf regression that was also introduced.

r? `@oli-obk`
cc `@fee1-dead`
@fee1-dead fee1-dead restored the const_env branch December 3, 2021 16:45
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 17, 2021
@bors
Auto merge of rust-lang#91354 - fee1-dead:const_env, r=spastorino
dd03c0f 
Cleanup: Eliminate ConstnessAnd

This is almost a behaviour-free change and purely a refactoring. "almost" because we appear to be using the wrong ParamEnv somewhere already, and this is now exposed by failing a test using the unstable `~const` feature.

We most definitely need to review all `without_const` and at some point should probably get rid of many of them by using `TraitPredicate` instead of `TraitRef`.

This is a continuation of rust-lang#90274.

r? `@oli-obk`

cc `@spastorino` `@ecstatic-morse`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 17, 2021
@spastorino
Revert "Auto merge of rust-lang#91354 - fee1-dead:const_env, r=spasto…
23f752f 
…rino"

This reverts commit 18bb8c6, reversing
changes made to d9baa36.
@fee1-dead fee1-dead added the F-const_trait_impl `#![feature(const_trait_impl)]` label Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spastorino spastorino spastorino approved these changes

Assignees

@spastorino spastorino

Labels
F-const_trait_impl `#![feature(const_trait_impl)]` merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.