Skip to content

compare generic constants using AbstractConsts - #76575

Merged
bors merged 12 commits into
rust-lang:masterfrom
lcnr:abstract-const
Sep 18, 2020
Merged

compare generic constants using AbstractConsts#76575
bors merged 12 commits into
rust-lang:masterfrom
lcnr:abstract-const

Conversation

@lcnr

@lcnrlcnr commented Sep 10, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

This is a MVP of rust-lang/compiler-team#340. The changes in this PR should only be relevant if feature(const_evaluatable_checked) is enabled.

currently based on top of #76559, so blocked on that.

r? @oli-obk cc @varkor@eddyb

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2020
@lcnrlcnr added A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` labels Sep 10, 2020
oli-obk
oli-obk requested changes Sep 10, 2020
View reviewed changes

@oli-obkoli-obk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At the current level this is still very reasonable to keep an overview about, but I think we should proactively document the AbstractConstBuilder in detail

Comment threadcompiler/rustc_middle/src/mir/abstract_const.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/mod.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/fulfill.rs Outdated

@oli-obkoli-obkSep 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe make the try_unify_abstract_consts query a raw query and add a wrapper that has a fast path for such checks

@oli-obk

oli-obk commented Sep 11, 2020

Copy link
Copy Markdown
Contributor

Does this treat

constX:usize = {let x = 4; x + 1};constY:usize = 4 + 1;

the same? Should it?

@lcnr

lcnr commented Sep 11, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

We do not yet support let bindings, as they add a StatementKind::FakeRead to the mir, added a test for this.

I think it's a good idea to not support them for now and see how often they are desired once this lands on nightly.

@lcnr
lcnrforce-pushed the abstract-const branch 2 times, most recently from b5d6616 to f7a7cf9CompareSeptember 11, 2020 20:12
@bors

bors commented Sep 12, 2020

Copy link
Copy Markdown
Collaborator

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

oli-obk
oli-obk reviewed Sep 18, 2020
View reviewed changes
Comment threadcompiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated

@oli-obkoli-obkSep 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We only need to build this in case we ever hit the situation where b_def != def || b_substs != substs. I understand that this only computes it once even if needed multiple times, but maybe it can be cached in a mutable local Option?

@oli-obkoli-obkSep 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, this is super cheap for !polymorphic const bodies... carry on

oli-obk
oli-obk reviewed Sep 18, 2020
View reviewed changes
Comment threadcompiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated

@oli-obkoli-obkSep 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I'm reading the code right, this is not needed as AbstractConst::new will return None if the feature gate is not enabled

@lcnrlcnrSep 18, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I kept it in as we otherwise have to reason globally here.

We would also be able to enter this conditional if we look at constants from extern crates if these crates use this feature without that check, which I don't actually want. (so that we can use it in libs without worrying about this)

@oli-obk

oli-obk commented Sep 18, 2020

Copy link
Copy Markdown
Contributor

I think this can go out of draft mode now? 😄

@lcnr
lcnr marked this pull request as ready for review September 18, 2020 15:13
@lcnr

lcnr commented Sep 18, 2020

Copy link
Copy Markdown
ContributorAuthor

@bors r=oli-obk rollup=never in case this influences perf

@bors

bors commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 09e6254 has been approved by oli-obk

@borsbors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2020
@borsbors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 18, 2020
@lcnr

lcnr commented Sep 18, 2020

Copy link
Copy Markdown
ContributorAuthor

@bors r=oli-obk

@bors

bors commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit b764120 has been approved by oli-obk

@bors

bors commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit b764120 with merge 9f8ac71...

@bors

bors commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 9f8ac71 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Sep 18, 2020
@bors
bors merged commit 9f8ac71 into rust-lang:masterSep 18, 2020
@rustbotrustbot added this to the 1.48.0 milestone Sep 18, 2020
@lcnr
lcnr deleted the abstract-const branch September 18, 2020 19:39
@lcnrlcnr mentioned this pull request Sep 19, 2020
Open
@ecstatic-morse

ecstatic-morse commented Sep 21, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Hi! This PR showed up in the weekly perf triage report. It seems to have caused a
moderate regression in instruction counts (up to 2.4% on full builds of inflate-check).

It seems like this code is only needed behind a feature gate. @lcnr is there anything that can be done to reduce the impact for stable users?

@lcnrlcnr mentioned this pull request Sep 22, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2020
@bors
Auto merge ofrust-lang#77041- lcnr:const-eval-perf, r=ecstatic-morse
521d8d8
perf: move cold path of `process_obligations` into a separate function
cc rust-lang#76575
This probably won't matter too much in the long run once rust-lang#69218 is merged so we may not want to merge this.
r? `@ecstatic-morse`
@lcnrlcnr added the F-generic_const_exprs `#![feature(generic_const_exprs)]` label Nov 14, 2020
@ehussehuss mentioned this pull request May 17, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obkoli-obkoli-obk requested changes

Assignees

@oli-obkoli-obk

Labels

A-const-genericsArea: const generics (parameters and arguments)F-const_generics`#![feature(const_generics)]`F-generic_const_exprs`#![feature(generic_const_exprs)]`merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

6 participants

@lcnr@oli-obk@bors@ecstatic-morse@rust-highfive@rustbot