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

Erase trivial caller-bounds when typechecking MIR after optimizations #94238

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 22, 2022

Fixes an ICE when inlining this code (with -Zmir-opt-level=3):

trait Factory<T> {
    type Item;
}

struct IntFactory;

impl<T> Factory<T> for IntFactory {
    type Item = usize;
}

fn foo<T>() where IntFactory: Factory<T> {
    let mut x: <IntFactory as Factory<T>>::Item = bar::<T>();
}

#[inline]
fn bar<T>() -> <IntFactory as Factory<T>>::Item {
    0usize
}

Specifically, inside of bar, we normalize <IntFactory as Factory<T>>::Item to usize, but we cannot do the same inside of foo, because param-env candidates have precedence over impl candidates when satisfying the projection. This usually isn't a problem, but becomes one when we inline bar into baz and cannot unify usize with <IntFactory as Factory<T>>::Item.

This attempts to solve the issue by filtering thru the ParamEnv and removing any predicates that are "trivial" -- that is, that are satisfied by the ParamEnv when the bound is removed from the ParamEnv's caller bounds. This ensures that we never allow a trivial <Ty as Trait> predicate in the ParamEnv interfere with projection normalization during codegen.


cc: #91743 (comment)
cc: @cjgillot since it helps a get rid of a few (but not all) ICEs seen from nalgebra-v0.21.1 in #91743

NOTE: this is kinda a hack, a super heavy hammer that I'm not sure if we should implement in the first place. I'm open to closing this piece of garbage out for a better brainstorming session for how to handle ambiguities as noted above. 😄

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 22, 2022
@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 Feb 22, 2022
@compiler-errors
Copy link
Member Author

compiler-errors commented Feb 22, 2022

r? @nikomatsakis since this has to do with how we normalize projection types, but feel free to send this to someone else

@oli-obk
Copy link
Contributor

oli-obk commented Feb 24, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 24, 2022
@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Trying commit 240e56189205b2a30bd84601cbcbede36b33812b with merge 689f51a2c72ad2d2c5eb3719fa78e8e209214e64...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 24, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2022
@compiler-errors
Copy link
Member Author

lol let me fix that

@tmiasko
Copy link
Contributor

tmiasko commented Feb 24, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Trying commit 244a73c with merge dc5b9515ae3599b4dc5438f23aee3092e95c2543...

@bors
Copy link
Contributor

bors commented Feb 24, 2022

☀️ Try build successful - checks-actions
Build commit: dc5b9515ae3599b4dc5438f23aee3092e95c2543 (dc5b9515ae3599b4dc5438f23aee3092e95c2543)

@rust-timer
Copy link
Collaborator

Queued dc5b9515ae3599b4dc5438f23aee3092e95c2543 with parent 3d127e2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dc5b9515ae3599b4dc5438f23aee3092e95c2543): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 24, 2022
@compiler-errors
Copy link
Member Author

@rustbot label -S-waiting-on-author

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 25, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Mar 10, 2022

param-env candidates have precedence over impl candidates when satisfying the projection.

could we flip this in reveal_all mode?

@compiler-errors
Copy link
Member Author

I could try that, shouldn't be too hard.

@nikomatsakis
Copy link
Contributor

Hmm, this seems like a good thing to talk over, @compiler-errors. If it works for you, maybe schedule something at https://calendly.com/nikomatsakis/ ?

@compiler-errors
Copy link
Member Author

So after discussing with @nikomatsakis, this PR is probably not the right way to go through with this. We discussed a few other options, which I'll likely test out and maybe put up for review. I'll mark this one as experimental.

@rustbot -S-waiting-on-review +S-experimental

@compiler-errors
Copy link
Member Author

lol whoops

@rustbot label -S-waiting-on-review +S-experimental

@rustbot rustbot added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2022
@compiler-errors
Copy link
Member Author

This is probably(?) fixed by @cjgillot's finished mir-inliner PR. I will add a test later anyways.

@compiler-errors compiler-errors deleted the bad-mir-bounds branch August 11, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants