-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Filter and test predicates using normalize_and_test_predicates
for const-prop
#68297
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ use rustc::mir::{ | |
SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind, | ||
UnOp, RETURN_PLACE, | ||
}; | ||
use rustc::traits::TraitQueryMode; | ||
use rustc::traits; | ||
use rustc::ty::layout::{ | ||
HasDataLayout, HasTyCtxt, LayoutError, LayoutOf, Size, TargetDataLayout, TyLayout, | ||
}; | ||
|
@@ -90,28 +90,28 @@ impl<'tcx> MirPass<'tcx> for ConstProp { | |
// If there are unsatisfiable where clauses, then all bets are | ||
// off, and we just give up. | ||
// | ||
// Note that we use TraitQueryMode::Canonical here, which causes | ||
// us to treat overflow like any other error. This is because we | ||
// are "speculatively" evaluating this item with the default substs. | ||
// While this usually succeeds, it may fail with tricky impls | ||
// (e.g. the typenum crate). Const-propagation is fundamentally | ||
// "best-effort", and does not affect correctness in any way. | ||
// Therefore, it's perfectly fine to just "give up" if we're | ||
// unable to check the bounds with the default substs. | ||
// We manually filter the predicates, skipping anything that's not | ||
// "global". We are in a potentially generic context | ||
// (e.g. we are evaluating a function without substituting generic | ||
// parameters, so this filtering serves two purposes: | ||
// | ||
// False negatives (failing to run const-prop on something when we actually | ||
// could) are fine. However, false positives (running const-prop on | ||
// an item with unsatisfiable bounds) can lead to us generating invalid | ||
// MIR. | ||
if !tcx.substitute_normalize_and_test_predicates(( | ||
source.def_id(), | ||
InternalSubsts::identity_for_item(tcx, source.def_id()), | ||
TraitQueryMode::Canonical, | ||
)) { | ||
trace!( | ||
"ConstProp skipped for item with unsatisfiable predicates: {:?}", | ||
source.def_id() | ||
); | ||
// 1. We skip evaluating any predicates that we would | ||
// never be able prove are unsatisfiable (e.g. `<T as Foo>` | ||
// 2. We avoid trying to normalize predicates involving generic | ||
// parameters (e.g. `<T as Foo>::MyItem`). This can confuse | ||
// the normalization code (leading to cycle errors), since | ||
// it's usually never invoked in this way. | ||
let predicates = tcx | ||
.predicates_of(source.def_id()) | ||
.predicates | ||
.iter() | ||
.filter_map(|(p, _)| if p.is_global() { Some(*p) } else { None }) | ||
.collect(); | ||
if !traits::normalize_and_test_predicates( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why call the function and not the query here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't realize this was a query, lol. EDIT: This isn't a query. Did you want me to query-ify this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well there's substitute_normalize_and_test_predicates which you used before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oli-obk: That would use all of the predicates, which I needed to filter beforehand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh... sorry for not reading the docs properly. You're totally right. Let's merge this PR for now and experiment with making it a query separately. Such a query could share a lot of evaluation time between the predicate elaborations of completely independent definitions if they have the same predicate list. |
||
tcx, | ||
traits::elaborate_predicates(tcx, predicates).collect(), | ||
) { | ||
trace!("ConstProp skipped for {:?}: found unsatisfiable predicates", source.def_id()); | ||
return; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// check-pass | ||
// compile-flags: --emit=mir,link | ||
// Regression test for issue #68264 | ||
// Checks that we don't encounter overflow | ||
// when running const-prop on functions with | ||
// complicated bounds | ||
pub trait Query {} | ||
|
||
pub trait AsQuery { | ||
type Query: Query; | ||
} | ||
pub trait Table: AsQuery + Sized {} | ||
|
||
pub trait LimitDsl { | ||
type Output; | ||
} | ||
|
||
pub(crate) trait LoadQuery<Conn, U>: RunQueryDsl<Conn> {} | ||
|
||
impl<T: Query> AsQuery for T { | ||
type Query = Self; | ||
} | ||
|
||
impl<T> LimitDsl for T | ||
where | ||
T: Table, | ||
T::Query: LimitDsl, | ||
{ | ||
type Output = <T::Query as LimitDsl>::Output; | ||
} | ||
|
||
pub(crate) trait RunQueryDsl<Conn>: Sized { | ||
fn first<U>(self, _conn: &Conn) -> U | ||
where | ||
Self: LimitDsl, | ||
Self::Output: LoadQuery<Conn, U>, | ||
{ | ||
// Overflow is caused by this function body | ||
unimplemented!() | ||
} | ||
} | ||
|
||
fn main() {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The overflow only happens if the current crate is a library. It does not happen if it's a binary crate (containing a main function). That means this test tests something different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing You can verify this locally with |
Uh oh!
There was an error while loading. Please reload this page.