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.

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

opportunistically evaluate constants while populating mir::Body::required_consts #71802

Open
oli-obk opened this issue May 2, 2020 · 9 comments
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2020

We can evaluate all constants that do not depend on generic parameters in

if let ConstKind::Unevaluated(_, _, _) = const_kind {

We make that a MutVisitor that evaluates all constants that it can and replaces them in-place and only adds those that it couldn't evaluate to the required_const list.

This way we never have to evaluate another constant in the entire MIR optimization pipeline, because we know none can be evaluated (modulo those that we already evaluate for merging required_consts during inlining.

cc @rust-lang/wg-mir-opt I think I like having the above invariant. It will make working with constants in MIR much simpler and make any kind of error handling that would have to be implemented unified into a single location.

@oli-obk oli-obk added A-const-eval Area: constant evaluation (mir interpretation) A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining labels May 2, 2020
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 2, 2020
@RalfJung
Copy link
Member

RalfJung commented May 2, 2020

and make any kind of error handling that would have to be implemented unified into a single location.

Ideally that would actually dispatch through the const_eval error handling to have it all centralized there...

Same goes for the const error reporting that codegen is doing, FWIW.

Which makes me wonder, can we move this impl block to librustc_mir/const_eval/error.rs or so? This is solely CTFE error logic, so logically part of the "CTFE <-> rustc glue" that we otherwise have in librustc_mir::const_eval.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 2, 2020

hmm right, I think nothing from librustc uses this anymore. We can't move the impl block, but we can make the functions free functions.

@RalfJung RalfJung added C-cleanup Category: PRs that clean code up or issues documenting cleanup. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels May 2, 2020
@RalfJung
Copy link
Member

RalfJung commented May 2, 2020

Yeah something like that.

So we also use this issue to track cleaning up the error reporting story more generally? If we also include cleaning up this then I think we can close #67191, AFAIK that's all that's left.

@RalfJung RalfJung changed the title opportunistically evaluate constants while populating mir::Body::required_consts opportunistically evaluate constants while populating mir::Body::required_consts + CTFE error cleanup May 2, 2020
@RalfJung
Copy link
Member

RalfJung commented May 3, 2020

rust-lang/miri#1382 is also related, this is Miri lacking the code that codegen has to properly report post-monomorphization errors.

@spastorino spastorino self-assigned this May 5, 2020
@RalfJung RalfJung changed the title opportunistically evaluate constants while populating mir::Body::required_consts + CTFE error cleanup opportunistically evaluate constants while populating mir::Body::required_consts Aug 12, 2020
@RalfJung
Copy link
Member

I realized that the cleanup here is not actually made easier by opportunistically evaluating things, because promoteds can still fail to evaluate later. Thus I moved the cleanup to a separate issue: #75461.

@tmiasko
Copy link
Contributor

tmiasko commented Jun 6, 2021

Evaluating and replacing constants in MIR before promotion interacts with const qualification. Evaluated constants are currently qualified based solely on their type, which is less precise than value based analysis that might be used otherwise. For example, promotion fails for X when it is evaluated in main (type contains interior mutability):

use std::cell::Cell;
const X: Option<Cell<i32>> = None;
fn main() {
    let x: &'static _ = &X;
}

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 6, 2021

Your example already compiles. We compute the qualification for every constant based on its body and then use that cached qualification to decide whether to promote references to it. Unless the unevaluated constant is too generic to get its true body (for associated consts of generic parameters), then we indeed just look at the type.

@tmiasko
Copy link
Contributor

tmiasko commented Jun 6, 2021

Evaluated constants are treated as opaque so the example stops to compile. This seems almost contradictory that we perform value based analysis only when we don't know the precise value yet.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 6, 2021

Oh wow. Now I get what you mean. Yea... I see how that would be problematic

@spastorino spastorino removed their assignment Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants