-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Revert the LazyConst
PR
#59178
Revert the LazyConst
PR
#59178
Conversation
src/librustc/mir/interpret/value.rs
Outdated
@@ -42,6 +43,10 @@ pub enum ConstValue<'tcx> { | |||
/// An allocation together with a pointer into the allocation. | |||
/// Invariant: the pointer's `AllocId` resolves to the allocation. | |||
ByRef(Pointer, &'tcx Allocation), | |||
|
|||
/// Used in the HIR by using `Unevaluated` everywhere and later normalizing to one of the other | |||
/// variants if the code is monomorphic enough for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should be at the top with Param
/ Infer
- but also, I wouldn't mind having Scalar
, Slice
and ByRef
in a sub-enum, because they are the "concrete values".
Maybe keep only Scalar
, Slice
and ByRef
here and move everything else to a ty::ConstKind
enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a change will cause major fallout and be very bitrotty. I can do that in a separate PR after const generics has been merged as to not block the const generics work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, I was just hoping to avoid having "known value" methods on the general type.
However, IIRC miri uses its own enum for values so this should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened as #59210
src/librustc/ty/sty.rs
Outdated
@@ -2276,6 +2230,10 @@ impl<'tcx> Const<'tcx> { | |||
} | |||
} | |||
} | |||
ConstValue::Unevaluated(..) => { | |||
flags |= TypeFlags::HAS_NORMALIZABLE_PROJECTION; | |||
flags |= TypeFlags::HAS_PROJECTION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to include flags from the substs, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically treat Unevaluated
like ty::{Adt,Projection,UnnormalizedProjection}
- unless they differ on something, in which case it needs to be looked at in more detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are included via the add_const
method that calls this method. All of that logic is slightly weirdly distributed. Not sure what's going on there. We should refactor these somewhat, but that's a preexisting issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'm confused as to why this Const::type_flags
method even exists, I'll look into it a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah looks like this is a mistake from the const generics work, mimicking RegionKind
(which idk why it has a type_flags
method but it's not great).
This should be inlined/moved into add_const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened #59209 to track it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I wanted you to do it here since it should be pretty easy (only one call site) and so I can actually review it. I can't tell very well if this is correct because it's awkwardly split into two.
r=me with nits fixed @bors p=77 |
@@ -411,8 +411,9 @@ impl<'a, 'b, 'gcx, 'tcx> TypeFolder<'gcx, 'tcx> for AssociatedTypeNormalizer<'a, | |||
}; | |||
if let Ok(evaluated) = tcx.const_eval(param_env.and(cid)) { | |||
let substs = tcx.lift_to_global(&substs).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do substs
need to be lifted here? If substs
needed inference but the const still evaluated successfully, doesn't that mean we can just use InternalSubsts::empty()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or am I misunderstanding what's happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing - InternalSubsts::empty()
would be wrong, but I'm wondering why there is any substitution at all, maybe this is from the time when this could return e.g. function pointers referring to generic parameters?
IMO const_eval
should either return an (interned) &'tcx ty::Const
, or the Scalar | Slice | ByRef
enum by value, not what it does now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nevermind, const_eval
returns the type as well, which could depend on parameters and whatnot (especially lifetime ones!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, cool, makes sense. The reason I ask is that this blows up on array lengths that use generic parameters when they have a real param_env.
@bors r=eddyb |
📌 Commit 336d42a8448417e7aa7cc6bd1eb925f2b9a296a8 has been approved by |
@bors r- |
Could you add some explanation about why this is getting reverted? |
I added an explanation to the main post |
@bors r=eddyb |
📌 Commit ae00a0f69f5caa8921cacec6a9953c3b12b85775 has been approved by |
@bors r-
EDIT: nevermind... I just messed up some copy pasting |
☔ The latest upstream changes (presumably #58140) made this pull request unmergeable. Please resolve the merge conflicts. |
⌛ Testing commit bb1d27b9653147f35e09f95dbc7847271201ec8d with merge a0334b6dbba8370396b7a01490dd28d13e59b502... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r- |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r=eddyb |
📌 Commit 5cd2806 has been approved by |
Revert the `LazyConst` PR The introduction of `LazyConst` did not actually achieve the code simplicity improvements that were the main reason it was introduced. Especially in the presence of const generics, the differences between the "levels of evaluatedness" of a constant become less clear. As it can be seen by the changes in this PR, further simplifications were possible by folding `LazyConst` back into `ConstValue`. We have been able to keep all the advantages gained during the `LazyConst` refactoring (like `const_eval` not returning an interned value, thus making all the `match` code simpler and more performant). fixes #59209 r? @eddyb @varkor
☀️ Test successful - checks-travis, status-appveyor |
} else { | ||
c.super_visit_with(self) | ||
} | ||
flags.intersects(self.flags) || c.super_visit_with(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the || ...
recursion part, since the flags should be complete.
|
||
&'tcx ty::LazyConst<'tcx> { | ||
match self { | ||
// FIXME(const_generics) this should print at least the type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You deleted this important comment - it should be wherever the Unevaluated
case ends up. cc @varkor
instantiated_predicates, | ||
location.to_locations(), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @nikomatsakis this code seems waay too specific (unless it applies to TerminatorKind::Call
callees?)
The introduction of
LazyConst
did not actually achieve the code simplicity improvements that were the main reason it was introduced. Especially in the presence of const generics, the differences between the "levels of evaluatedness" of a constant become less clear. As it can be seen by the changes in this PR, further simplifications were possible by foldingLazyConst
back intoConstValue
. We have been able to keep all the advantages gained during theLazyConst
refactoring (likeconst_eval
not returning an interned value, thus making all thematch
code simpler and more performant).fixes #59209
r? @eddyb @varkor