-
Notifications
You must be signed in to change notification settings - Fork 13.5k
GCI: Don't evaluate the initializer of free const items that have trivially unsatisfied predicates #142293
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
base: master
Are you sure you want to change the base?
Conversation
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[early rough draft] GCI: Don't evaluate free const items with impossible predicates
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
We should consider moving all of these call-site checks into ctfe itself, e.g. eval_to_const_value_raw_provider
. This wont affect const patterns which go through _for_typeck
versions of the CTFE entry points.
I also then think that the impossible predicates check in that place should ICE on predicates containing non region params as that shouldn't be reachable (and if it is, then we're evaluating generic constants in places we might not want to be).
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (4151b9a): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -5.1%, secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 753.145s -> 753.291s (0.02%) |
Cargo.lock
Outdated
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 also then think that the impossible predicates check in that place should ICE on predicates containing non region params as that shouldn't be reachable (and if it is, then we're evaluating generic constants in places we might not want to be).
Adding debug_assert!(!predicates.has_non_region_param());
to fn impossible_predicates
, lead to a whole slew of test failures (I didn't complete the test run). E.g.,
Some failing tests
tests/ui/associated-consts/associated-const-marks-live-code.rs
tests/ui/async-await/async-fn-size-moved-locals.rs
tests/ui/async-await/async-fn-size-uninit-locals.rs
tests/ui/async-await/repeat_count_const_in_async_fn.rs
tests/ui/binding/match-arm-statics.rs
tests/ui/const_prop/ice-issue-96944.rs
tests/ui/consts/const-autoderef.rs
tests/ui/consts/chained-constants-stackoverflow.rs
tests/ui/consts/const-blocks/const-repeat.rs
tests/ui/consts/const-const.rs
tests/ui/consts/const-enum-cast.rs
tests/ui/consts/const-eval/strlen.rs
tests/ui/consts/const-expr-in-fixed-length-vec.rs
tests/ui/consts/const-expr-in-vec-repeat.rs
tests/ui/consts/const-fn-method.rs
tests/ui/consts/const-trait-to-trait.rs
tests/ui/consts/ice-48279.rs
tests/ui/consts/min_const_fn/min_const_fn_libstd.rs
tests/ui/consts/offset_from.rs
tests/ui/consts/ptr_comparisons.rs
tests/ui/consts/ptr_is_null.rs
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.
Adding
debug_assert!(!predicates.has_non_region_param());
to fnimpossible_predicates
I was wrong about that, it's actually completely fine 🤦 I must've had some other changes in the working tree... Sry bout the miscommunication there
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.
We should consider moving all of these call-site checks into ctfe itself, e.g.
eval_to_const_value_raw_provider
. This wont affect const patterns which go through_for_typeck
versions of the CTFE entry points.
As discussed, long term, that would be the way to go. However, it makes me uncomfortable that eval_to_const_value_raw
has so many transitive callers which I wasn't yet able to audit entirely. I know that you told me that this function should be safe to modify from a soundness perspective and you're probably right, I just haven't finished tracing the entire caller graph. WIP:
Caller graph (X called by Y)
+ eval_to_const_value_raw [eval_to_const_value_raw_provider] (???)
+-- check_crate/par_hir_body_owners/DefKind::Const (OK)
+-- const_eval_global_id (???)
+-- eval_intrinsic (OK)
+-- const_eval_poly (OK)
| +-- codegen_global_asm (OK)
| +-- check_type_defn/variants/fields [default_field_values] (OK)
| +-- check_type_defn/ty::VariantDiscr::Explicit (OK)
| +-- eval_explicit_discr (OK)
| +-- monomorphize/RootCollector/process_item/DefKind::Const (OK)
+-- const_eval_resolve (???)
| +-- Const::eval (???)
| +-- eval_mir_constant (???)
| | +... [~20 call sites] (TODO)
| +-- inline_to_global_operand/InlineAsmOperand::Const (OK)
| +-- push_stack_frame_raw/body.required_consts() (???)
| +-- try_eval_scalar... (TODO)
| +-- MIR transform/simplify_repeated_aggregate (???)
| +-- eval_constant (???)
| +-- eval_operand... (TODO)
| +-- MIR transform/ConstPropagator/visit_const_operand (???)
| +-- collect_items_of_instance/body.requires_consts() (???)
+-- const_eval_instance (???)
+-- codegen_regular_intrinsic_call (OK)
+-- codegen_intrinsic_call (OK)
+-- eval_instance (???)
+-- try_const_eval [SMIR] (???)
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.
(As an aside, even if we enriched eval_to_const_value_raw_provider
with a call to impossible_predicates
, here in mod reachable
it would be of no use to us as it uses const_eval_poly_to_alloc
which doesn't depend on that function)
498b2e2
to
380088d
Compare
Mono collector now uses Despite that, imma recheck perf cuz I removed the |
This comment has been minimized.
This comment has been minimized.
[early rough draft] GCI: Don't evaluate free const items with impossible predicates
if !self.tcx.generics_of(def_id).own_requires_monomorphization() | ||
&& !self.tcx.instantiate_and_check_impossible_predicates(( | ||
def_id, | ||
ty::GenericArgs::identity_for_item(self.tcx, def_id), |
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 should probably just copy the logic from DefKind::Enum/Struct/Union
for checking impossible preds and constructing identity args
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.
The const item branch now copies the relevant parts of the ADT branch 👍
let predicates = tcx.predicates_of(item_def_id); | ||
let predicates = predicates.instantiate_identity(tcx).predicates; | ||
|
||
// FIXME(fmease): May its result be 'incorrect' if the preds have ReEarlyBound? |
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.
Yeah I think you should be able to construct a case where an impossible pred isnt found due to a lifetime parameter. E.g. const _: usize where for<'b> [&'a u8]: Sized = ...
probably will get evaluated unless you erase regions
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.
We're now erasing in mono collector and as a consequence we no longer eval the initializer in your example 👍
|
||
// FIXME(fmease): May its result be 'incorrect' if the preds have ReEarlyBound? | ||
if !traits::impossible_predicates(tcx, predicates) { | ||
// FIXME(generic_const_items): Passing empty instead of identity args is fishy but |
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 should be fixed yeah 🤔 I don't think ctfe should have to handle there being instances with wrong args
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.
Right, we should. If you're okay with it, I'd prefer to address this in a separate PR because this is super perf sensitive.
This used to be a call to fn const_eval_poly
for the longest time which used to construct identity args etc. After a reshuffle (#121087), it was changed to the current no-args fully-mono version (#121387) to fix perf. Maybe the identity vs. no args isn't mega perf sensitive and it's paramenv norm that's the culprit but in #136429 I also did some tests, so it's unclear.
E.g., ea1cdda (perf results: #136429 (comment)) and 6f11489 (perf results: #136429 (comment)). Ofc, they also include unrelated changes to WF-check, so the results are skewed.
GCI: Don't evaluate the initializer of free const items that have trivially unsatisfied predicates
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (cea506b): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient 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 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 690.402s -> 687.107s (-0.48%) |
This comment was marked as resolved.
This comment was marked as resolved.
0e9126c
to
8d3bc94
Compare
PR #142893 (which conflicted with this PR / made it unmergeable) heavily affected perf. Thus I'm now retrying if this PR is still sensitive. @bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
GCI: Don't evaluate the initializer of free const items that have trivially unsatisfied predicates Part of #113521.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (41283df): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.5%, secondary 8.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 462.452s -> 460.902s (-0.34%) |
8d3bc94
to
569f3a3
Compare
569f3a3
to
3cddea4
Compare
|
||
// FIXME(fmease): May its result be 'incorrect' if the preds have ReEarlyBound? | ||
if !traits::impossible_predicates(tcx, predicates) { | ||
// FIXME(generic_const_items): Passing empty instead of identity args is fishy but |
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.
Right, we should. If you're okay with it, I'd prefer to address this in a separate PR because this is super perf sensitive.
This used to be a call to fn const_eval_poly
for the longest time which used to construct identity args etc. After a reshuffle (#121087), it was changed to the current no-args fully-mono version (#121387) to fix perf. Maybe the identity vs. no args isn't mega perf sensitive and it's paramenv norm that's the culprit but in #136429 I also did some tests, so it's unclear.
E.g., ea1cdda (perf results: #136429 (comment)) and 6f11489 (perf results: #136429 (comment)). Ofc, they also include unrelated changes to WF-check, so the results are skewed.
/// Evaluates a constant without providing any generic parameters. | ||
/// | ||
/// This is useful to evaluate consts that can't take any generic arguments like enum | ||
/// discriminants. If a non-region generic parameter is used within the constant |
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.
(apart splitting of the synopsis / introductory paragraph, here & below I changed generic parameter
to non-region generic parameter
and dropped the mention of const items since they can be generic, GCI duh)
let predicates = tcx.predicates_of(item_def_id); | ||
let predicates = predicates.instantiate_identity(tcx).predicates; | ||
|
||
// FIXME(fmease): May its result be 'incorrect' if the preds have ReEarlyBound? |
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.
We're now erasing in mono collector and as a consequence we no longer eval the initializer in your example 👍
if !self.tcx.generics_of(def_id).own_requires_monomorphization() | ||
&& !self.tcx.instantiate_and_check_impossible_predicates(( | ||
def_id, | ||
ty::GenericArgs::identity_for_item(self.tcx, def_id), |
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.
The const item branch now copies the relevant parts of the ADT branch 👍
if tcx.generics_of(def_id).own_requires_monomorphization() { | ||
return; | ||
} | ||
let args = ty::GenericArgs::for_item(tcx, def_id, |param, _| { | ||
expect_and_erase_regions(tcx, param) | ||
}); | ||
if tcx.instantiate_and_check_impossible_predicates((def_id, args)) { | ||
return; | ||
} |
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 pattern occurs 3 times in this file: For ADTs, const items and impl blocks. However, I was unable to extract the code into a meaningful function. The only thing I was able to come up with was sth. like:
fn instantiate_erasing_regions_if_monomorphic_enough_and_check_impossible_predicates<'tcx>(
tcx: TyCtxt<'tcx>,
def_id: DefId,
) -> Option<ty::GenericArgs<'tcx>> {
if !tcx.generics_of(def_id).requires_monomorphization(tcx)
&& let args =
ty::GenericArgs::for_item(tcx, def_id, |param, _| expect_and_erase_regions(tcx, param))
&& !tcx.instantiate_and_check_impossible_predicates((def_id, args))
{
return Some(args);
}
None
}
and my eyes started bleeding 🩸🩸🩸
Cargo.lock
Outdated
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.
Adding
debug_assert!(!predicates.has_non_region_param());
to fnimpossible_predicates
I was wrong about that, it's actually completely fine 🤦 I must've had some other changes in the working tree... Sry bout the miscommunication there
@rustbot review |
@@ -238,12 +238,18 @@ pub fn check_crate(tcx: TyCtxt<'_>) { | |||
check::maybe_check_static_with_link_section(tcx, item_def_id); | |||
} | |||
DefKind::Const if !tcx.generics_of(item_def_id).own_requires_monomorphization() => { |
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 would expect !own_requires_monomorphization
means there are no generics to begin with? Or what does that function mean? It is sadly undocumented...
It does not make sense to me that we'd ever evaluate a generic free const item. How would we even pick the instance for the generic parameters...?
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, there was #136429 which I already forgot about. IMO the obvious fix is to just revert that PR...
... but I guess that wouldn't even be enough since even non-generic consts can have where clauses. Right. Uh, we may have to just skip all of those?
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.
own_requires_monomorphization
is synonymous with "has non-region generic params" meaning we do eval lifetime-generic const exprs (more precisely early-bound-lifetime-generic).
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.
How would we even pick the instance for the generic parameters...?
That's obviously fine since we're only dealing with lifetime params since trait resolution etc. happens modulo non-late-bound regions / most if not all regions get fully erased. No problem there.
let predicates = tcx.predicates_of(item_def_id); | ||
let predicates = predicates.instantiate_identity(tcx).predicates; | ||
|
||
if !traits::impossible_predicates(tcx, predicates) { |
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.
Is it guaranteed that !impossible
means "possible"? Or at least, does the contract for impossible_predicates
imply that a predicate considered "possible" will not ICE later when one does normal type system things and makes basic assumptions about primitive MIR ops, sizedness, and things like that?
(I feel I asked @BoxyUwU that same question already earlier this year in a different context and the answer was "yes it's fine", but I'm not sure...)
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.
Unfortunately I can't answer that with 100% confidence since I'm not super familiar with the innards and all the little details of const eval.
I know that layout computation has been made more robust against that somewhat recently but I'm not sure about the rest.
I know that the MIR pass ImpossiblePredicates
replaces MIR bodies with Unreachable
1 if the body has unsatisfiable ("trivially unsatisfied") global2 predicates. However, I don't actually know how early that pass gets run (and it doesn't affect e.g., layout computation unless I'm totally off base).
It's certainly a valid question and I need to have researched that before opening the RFC. Tho if my tired brain doesn't trick me right now, my change should in theory eliminate potential ICEs rather than introducing new ones because we eval less.
Footnotes
-
These actually trigger during const eval on master. ↩
-
Meaning it disagrees with the 3 checks I've added in this PR when there are predicates containing ReEarlyBound (erasing wouldn't help because ReErased also turns a predicate non-global). My new checks should be stricter than the one used during said MIR pass, so that shouldn't be problematic (or at least not more problematic compared to master). ↩
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.
my change should in theory eliminate potential ICEs rather than introducing new ones because we eval less.
Footnotes
Yeah, definitely. I was just wondering if it evals less enough. ;)
Part of #113521.