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

Implement (but don't use) valtree and refactor in preparation of use #82936

Merged
merged 27 commits into from
Mar 17, 2021

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 9, 2021

This PR does not cause any functional change. It refactors various things that are needed to make valtrees possible. This refactoring got big enough that I decided I'd want it reviewed as a PR instead of trying to make one huge PR with all the changes.

cc @rust-lang/wg-const-eval on the following commits:

  • 2027184 implement valtree
  • eeecea9 fallible Scalar -> ScalarInt
  • 042f663 ScalarInt convenience methods

cc @eddyb on ef04a6d

cc @rust-lang/wg-mir-opt for cf1700c (mir::Constant can now represent either a ConstValue or a ty::Const, and it is totally possible to have two different representations for the same value)

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid added the A-const-eval Area: Constant evaluation (MIR interpretation) label Mar 9, 2021
compiler/rustc_middle/src/mir/interpret/value.rs Outdated Show resolved Hide resolved
@@ -2012,7 +2013,7 @@ impl<'tcx> Operand<'tcx> {
Operand::Constant(box Constant {
span,
user_ty: None,
literal: ty::Const::zero_sized(tcx, ty),
literal: ConstantSource::Ty(ty::Const::zero_sized(tcx, ty)),
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you using ConstantSource::Ty instead of Val here?

do you prefer ty::Const values over allocations where possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly in order to change as little as possible. ConstantSource::Val is essentially dead code in this PR

compiler/rustc_middle/src/mir/type_foldable.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Mar 9, 2021

it's kinda unfortunate that ConstantSource has some duplication between it's variants, changing it to something like

enum ConstantSource {
    Param(ParamConst),
    Bound(...),
    Unevaluated(...),
    Value(...),
    Err(...),
}

would be nice, but it's also a bit annoying because we have to somehow change this for the subst folder to work correctly.
using normalize to evaluate the Unevaluated won't work anyways cause that evaluation might not return something representable as a val tree

probably missed/forgot some previous discussion about this

@bors
Copy link
Contributor

bors commented Mar 10, 2021

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 10, 2021

some duplication between it's variants, changing it to something like

you mean the two occurrences of ConstValue? One of them (the one in ty::Const will become ValTree, but you are right, we will keep having that duplication).

but it's also a bit annoying because we have to somehow change this for the subst folder to work correctly.

Yea, I have a local branch where I tried making ty::Const generic over ValTree or ConstValue to resolve this, but this is way too invasive to do right now. If we do this PR, then we get there in smaller steps. Yes the MIR isn't nice because you have two different representations for the same concrete constant, but the MIR already has that right now (ByRef can technically represent scalars).

The reason this PR is a better first step is that we have to handle all special cases around mir::Constant already without getting the hit on the "make everything generic" solution. Of course we could also "just" have fold_constant in addition to fold_const, but then we'll also get a lot of duplication that I don't know how to handle properly yet (like do we even need Infer, Bound and Param for mir::Constant?). We can explore all these questions with runtime checks before doing the hard thing and cleaning it all up.

@lcnr
Copy link
Contributor

lcnr commented Mar 12, 2021

would be r=me

@matthewjasper want to still look over this?

valtree is a version of constants that is inherently safe to be used within types.
This is in contrast to ty::Const which can have different representations of the same value. These representation differences can show up in hashing or equality comparisons, breaking type equality of otherwise equal types.
valtrees do not have this problem.
Value trees won't have scalar ptr at all, so we need a scalar int printing method anyway. This way we'll be able to share that method between all const representations.
This is in preparation of the `literal` field becoming an enum that distinguishes between type level constants and runtime constants
It's not necessary yet, but it may become necessary with things like lazy normalization.
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 16, 2021

@bors r=RalfJung,lcnr,matthewjasper

@bors
Copy link
Contributor

bors commented Mar 16, 2021

📌 Commit c4d564c has been approved by RalfJung,lcnr,matthewjasper

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

bors commented Mar 16, 2021

⌛ Testing commit c4d564c with merge e655fb6...

@bors
Copy link
Contributor

bors commented Mar 17, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung,lcnr,matthewjasper
Pushing e655fb6 to master...

@rylev
Copy link
Member

rylev commented Mar 23, 2021

@oli-obk @lcnr This seems to have regressed performance slightly in the ctfe stress benchmarks. The regression seems to happen in the eval_to_allocation_raw query. If this change is purely adding unused code, perhaps some inlining has been impacted in the refactoring?

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 25, 2021

cachegrind diff:

+1,933,323,286  ???:rustc_mir::interpret::operand::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::const_val_to_op
-1,352,146,900  ???:rustc_mir::interpret::operand::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::const_to_op
+452,987,692  ???:rustc_middle::ty::normalize_erasing_regions::<impl rustc_middle::ty::context::TyCtxt>::subst_and_normalize_erasing_regions
+227,857,068  ???:rustc_mir::interpret::operand::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_operand
+158,899,167  ???:rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::run
+155,194,634  ???:<rustc_middle::ty::layout::LayoutCx<rustc_middle::ty::query::TyCtxtAt> as rustc_target::abi::LayoutOf>::layout_of
+17,034,883  ???:<core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::try_fold
-14,942,211  ???:<rustc_middle::ty::normalize_erasing_regions::NormalizeAfterErasingRegionsFolder as rustc_middle::ty::fold::TypeFolder>::fold_const
-7,340,200  ???:rustc_middle::mir::interpret::value::ConstValue::try_to_bits
+3,932,250  ???:rustc_middle::mir::interpret::value::ConstValue::try_to_machine_usize

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) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.