Add rigid alias marker#156742
Conversation
|
Unfinished. Let's have a look at the perf impact nonetheless. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[Experiment] Add rigid alias marker
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 73eccb5 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[Experiment] Add rigid alias marker
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (42e2939): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.6%, secondary -1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.5%, secondary 17.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 510.83s -> 514.725s (0.76%) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
went through all changes myself, and while there's a bunch of future cleanup, I'd like to get this merged and work from there. Let's do another quick perf run and @BoxyUwU was open to look over this as well @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| *self.tcx, | ||
| self.typing_env, | ||
| ty::EarlyBinder::bind(value), | ||
| ty::EarlyBinder::bind(self.tcx.tcx, value), |
There was a problem hiding this comment.
| }), | ||
| ty, | ||
| // FIXME(rigid_aliases_marker): We have alias relating in | ||
| // `extract_verify_if_eq` which checks rigidness equality. |
There was a problem hiding this comment.
what does "checks rigidness equality" mean :3
| let verify_if_eq = verify_if_eq_b.skip_binder(); | ||
| m.relate(verify_if_eq.ty, test_ty).ok()?; | ||
| // FIXME(rigid_aliases_marker): Both types should be a rigid alias in all cases | ||
| // with the new solver here. For now we just force things to non-rigid. |
There was a problem hiding this comment.
why force things to non-rigid? is it not actually the case that its always rigid here?
| // FIXME(rigid_aliases_marker): The aliases in `erased_ty` are currently not | ||
| // marked as rigid. We should probably do that instead as we only ever want to | ||
| // prove `TypeOutlives` once all aliases in them have been normalized. | ||
| let outlives_ty = ty::set_aliases_to_non_rigid(tcx, outlives_ty).skip_normalization(); |
There was a problem hiding this comment.
why set them to non rigid :3 the erased_ty ones aren't but why does that mean we need outlives_ty to be non rigid too?
| // is gone. We set the whole type to non-rigid to be consistent with what we do in | ||
| // `can_match_erased_ty`. | ||
| let erased_alias_ty = self.tcx.erase_and_anonymize_regions( | ||
| ty::set_aliases_to_non_rigid(self.tcx, alias_ty) |
There was a problem hiding this comment.
why are we setting aliases to non rigid
| } | ||
|
|
||
| ty::Adt(_, _) | ty::Tuple(_) | ty::Array(_, _) | ty::Alias(_) | ty::Param(_) => { | ||
| // FIXME: This should not treat aliases this way. |
There was a problem hiding this comment.
not an important enough fixme to track this somewhere?
| }, | ||
| ) | ||
| | ty::Placeholder(..) | ||
| | ty::Alias(ty::IsRigid::No, _) |
There was a problem hiding this comment.
where do we actually ensure that everything is normalized before calling into the variety of methods in this module :3
There was a problem hiding this comment.
calls to structurally_normalize_ty before doing anything with them; the existing lazy normalization infrastructure
| let predicates = ocx.normalize(&cause, elaborated_env, Unnormalized::new_wip(predicates)); | ||
| // FIXME: opaque types in param env might be in defining scope but we're | ||
| // using non body analysis for here. So the rigidness marker is wrong. | ||
| let predicates = ty::set_aliases_to_non_rigid(tcx, predicates).skip_norm_wip(); |
There was a problem hiding this comment.
i guess u just have to be kinda careful when doing funny stuff with changing envs and we don't really have any guardrails?
There was a problem hiding this comment.
yeah, changing TypingEnvs and adding stuff to the ParamEnv both need to manually handle rigid aliases (which they always have/had to do with eager norm)
I don't really know how to avoid that, I think TypingEnv changes are rare enough that this is "fine" and these are very subtle regardless of rigid aliases, so adding more requirements to them seems barely acceptable to me.
Extending the ParamEnv will be interesting, but we should only do it in a single place in the trait solver, which would then be response to handle the aliases properly.
| // Currently we can't guarantee that. | ||
| if let Ok(_) = relation.relate( | ||
| alias.to_ty(infcx.cx(), IsRigid::No), | ||
| set_aliases_to_non_rigid(infcx.cx(), alias2).skip_norm_wip(), |
There was a problem hiding this comment.
what effect does this actually have 🤔
| Err(TypeError::ConstMismatch(ExpectedFound::new( | ||
| Const::new_unevaluated(cx, a), | ||
| Const::new_unevaluated(cx, b), | ||
| Const::new_unevaluated(cx, ty::IsRigid::yes_if_next_solver(cx), a), |
There was a problem hiding this comment.
we just assume these are normalized?
There was a problem hiding this comment.
yeah, we should never structurally relate aliases and fail unless the alias is rigid.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (6fcc74a): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.7%, secondary 10.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.4%, secondary -4.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 504.341s -> 508.657s (0.86%) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors r+ rollup=never p=1 |
|
@bors r- will conflict with the currently happening rollup |
|
This pull request was unapproved. |
View all comments
This PR adds a rigidness marker to
TyKind::AliasandConstKind::Unevaluated. It's used to skip renormalization of rigid aliases. The tracking issue for this is #155345.The difficulty is that rigid aliases are only valid in their own
TypingEnvso we need to force them back to non-rigid when entering anotherTypingEnv, either because a change to theParamEnvor because we enter anotherTypingMode.Changes to the
ParamEnvcurrently only happen by moving into a new context. We now make sure thatEarlyBindernew contains any rigid aliases, this way we have to normalize all aliases contained in it after instantiating.Changes to the
TypingModeare rare, and have to be manually handled.The main changes in this PR are as follows:
enum IsRigid { Yes, No }as a field toTyKind::AliasandConstKind::Unevaluated. It is alwaysNowith the old solver, this makes some of the code less nice than it will be with the old solver removed.Projectiongoals we equate the expected term with that alias withIsRigid::YesEarlyBinder::bindnow takes thetcxand eagerly sets allIsRigidtoNo, this is necessary as moving aliases into a differentTypingEnvmay cause them to no longer be rigidEarlyBinder::bind_iterasserts that there are no rigid aliases instead of replacing themTyKind::AliaswithIsRigid::NoThere's a lot of future work here:
AliasRelate, lazily replace non-rigid aliases with infer var + projection goalParamEnvnormalization hack to incorrectly mark things as rigidTypeOutlivesgoal handling to only expect rigid aliasesIsRigid::Yesin region handling (oryes_if_next_solver)EarlyBinder::bindto also assert that there are no rigid aliases and manually replacing rigid aliases before then where necessaryThis PR also adds a flag
-Zrenormalize-rigid-aliasesto test whether we properly handle typing mode change.Currently only
tests/ui/traits/next-solver/assembly/ambiguity-due-to-uniquification-4.rsfails this check.r? lcnr