Add query to check for impossible predicates, use it to skip `NoopMethodCall` and `Inline` #95398

compiler-errors · 2022-03-28T05:10:42Z

Fix a few cases where impossible founds can be found and can cause ICEs in codegen/linting.

Fixes #94999
Fixes #94680
Actually fixes #93008 this time

rust-highfive · 2022-03-28T05:10:46Z

(rust-highfive has picked a reviewer for you, use r? to override)

bors · 2022-03-31T01:04:39Z

☔ The latest upstream changes (presumably #95436) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot

The query you created makes it seem like having impossible predicates is a property of an item. Should this check be done earlier, ie. at the beginning of typeck, in order to skip all the work of type checking and MIR building?

cjgillot · 2022-04-01T15:09:40Z

compiler/rustc_middle/src/query/mod.rs

+    // 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.
+    query check_impossible_predicates_for_item(key: DefId) -> bool {


Could you rename the query to make the bool more explicit?
Something like item_has_impossible_predicates?

cjgillot · 2022-04-01T15:09:59Z

compiler/rustc_middle/src/query/mod.rs

+    // This branch will never be taken for any normal function.
+    // However, it's possible to `#!feature(trivial_bounds)]` to write
+    // a function with impossible to satisfy clauses, e.g.:
+    // `fn foo() where String: Copy {}`


This paragraph is outdated.

compiler-errors · 2022-04-01T19:17:03Z

@cjgillot:

The query you created makes it seem like having impossible predicates is a property of an item. Should this check be done earlier, ie. at the beginning of typeck, in order to skip all the work of type checking and MIR building?

Yes, I agree that it's a property of the where clause on an item. However, they are allowed on the item with feature(trivial_bounds) and even in some cases without that feature (which, as a side note, I am probably going to put up a PR to warn on the latter cases).

The current behavior is to typecheck the items as if these predicates are true. I think we should at least continue to typecheck the items given the false predicates -- though I wouldn't be opposed to skipping mir-build (or at least all of mir-opt) in the case that they have impossible predicates.

This PR doesn't currently do that because I wanted to keep the affected compiler surface small, since it's the inliner that I have seen with the most ICEs due to calling codegen_fulfill_obligation on impossible predicates. Do you mind if I investigate additional changes (e.g. to skip all of MIR) in a follow-up PR?

bors · 2022-04-03T11:13:30Z

☔ The latest upstream changes (presumably #85321) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2022-04-14T18:55:31Z

@rustbot author

compiler-errors · 2022-04-21T04:05:44Z

@rustbot ready

r? rust-lang/compiler

jackh726 · 2022-04-22T17:48:11Z

compiler/rustc_trait_selection/src/traits/mod.rs

    let mut predicates = tcx.predicates_of(key.0).instantiate(tcx, key.1).predicates;
    predicates.retain(|predicate| !predicate.needs_subst());
    let result = impossible_predicates(tcx, predicates);


It's surprising to me that this doesn't just delegate to item_has_impossible_predicates_for_item.

The behavior these two are slightly different; could they be merged?

I tried to before, but I had some regressions.

I think I could try again if I'm smarter with some things though... I'll investigate this weekend.

Do you think we should land this in the meantime? If so, I think I'd like a FIXME added, but other than that, r=me

It's not ridiculously critical, so I'll play around with it this weekend and either r=you with a better description of why they're different, or unify these two.

compiler-errors · 2022-04-24T21:57:35Z

@bors try @rust-timer queue

rust-timer · 2022-04-24T21:57:37Z

Awaiting bors try build completion.

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

bors · 2022-04-24T21:57:43Z

⌛ Trying commit 41ad624 with merge c55a268fc101a6e678d0894680e56e83673f0993...

bors · 2022-04-24T23:19:35Z

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

rust-timer · 2022-04-24T23:19:37Z

Queued c55a268fc101a6e678d0894680e56e83673f0993 with parent 18f314e, future comparison URL.

rust-timer · 2022-04-25T00:35:23Z

Finished benchmarking commit (c55a268fc101a6e678d0894680e56e83673f0993): comparison url.

Summary:

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	40	12	0	2	40
mean²	0.5%	1.4%	N/A	-1.2%	0.5%
max	0.9%	4.2%	N/A	-1.2%	0.9%

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 may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

compiler-errors · 2022-04-25T05:51:13Z

Oof, that perf regression sucks. It's possibly because I'm elaborating the predicates always.

Testing the previous implementation for perf regression in #96382.

compiler-errors · 2022-06-11T21:24:21Z

This was superseded by #96806

rust-highfive assigned matthewjasper Mar 28, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2022

compiler-errors force-pushed the impossible-preds branch from 825eafa to 9436ebe Compare March 31, 2022 02:42

cjgillot reviewed Apr 1, 2022

View reviewed changes

compiler-errors force-pushed the impossible-preds branch from 9436ebe to eb164bf Compare April 7, 2022 02:24

rustbot 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 Apr 14, 2022

compiler-errors mentioned this pull request Apr 19, 2022

[WIP] reject more impossible trivial bounds (HRTBs and trivial after normalization) #95611

Closed

compiler-errors force-pushed the impossible-preds branch from eb164bf to ed81aa6 Compare April 21, 2022 04:05

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 21, 2022

rust-highfive assigned jackh726 and unassigned matthewjasper Apr 21, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2022

jackh726 reviewed Apr 22, 2022

View reviewed changes

Check impossible predicates, use it to skip NoopMethodCall and Inline

6fd51e7

compiler-errors force-pushed the impossible-preds branch from ed81aa6 to 6fd51e7 Compare April 24, 2022 21:34

This comment has been minimized.

Sign in to view

appease clippy

41ad624

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 24, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 25, 2022

compiler-errors mentioned this pull request Apr 25, 2022

[PERF] Check previous impossible preds implementation #96382

Closed

jackh726 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 May 16, 2022

compiler-errors closed this Jun 11, 2022

compiler-errors deleted the impossible-preds branch August 11, 2023 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add query to check for impossible predicates, use it to skip `NoopMethodCall` and `Inline` #95398

Add query to check for impossible predicates, use it to skip `NoopMethodCall` and `Inline` #95398

compiler-errors commented Mar 28, 2022

rust-highfive commented Mar 28, 2022

bors commented Mar 31, 2022

cjgillot left a comment

cjgillot Apr 1, 2022

cjgillot Apr 1, 2022

compiler-errors commented Apr 1, 2022

bors commented Apr 3, 2022

compiler-errors commented Apr 14, 2022

compiler-errors commented Apr 21, 2022

jackh726 Apr 22, 2022

compiler-errors Apr 22, 2022

jackh726 Apr 22, 2022

compiler-errors Apr 22, 2022

This comment has been minimized.

compiler-errors commented Apr 24, 2022

rust-timer commented Apr 24, 2022

bors commented Apr 24, 2022

bors commented Apr 24, 2022

rust-timer commented Apr 24, 2022

rust-timer commented Apr 25, 2022

compiler-errors commented Apr 25, 2022

compiler-errors commented Jun 11, 2022

Add query to check for impossible predicates, use it to skip NoopMethodCall and Inline #95398

Add query to check for impossible predicates, use it to skip NoopMethodCall and Inline #95398

Conversation

compiler-errors commented Mar 28, 2022

rust-highfive commented Mar 28, 2022

bors commented Mar 31, 2022

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Apr 1, 2022

Choose a reason for hiding this comment

cjgillot Apr 1, 2022

Choose a reason for hiding this comment

compiler-errors commented Apr 1, 2022

bors commented Apr 3, 2022

compiler-errors commented Apr 14, 2022

compiler-errors commented Apr 21, 2022

jackh726 Apr 22, 2022

Choose a reason for hiding this comment

compiler-errors Apr 22, 2022

Choose a reason for hiding this comment

jackh726 Apr 22, 2022

Choose a reason for hiding this comment

compiler-errors Apr 22, 2022

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors commented Apr 24, 2022

rust-timer commented Apr 24, 2022

bors commented Apr 24, 2022

bors commented Apr 24, 2022

rust-timer commented Apr 24, 2022

rust-timer commented Apr 25, 2022

Footnotes

compiler-errors commented Apr 25, 2022

compiler-errors commented Jun 11, 2022

Add query to check for impossible predicates, use it to skip `NoopMethodCall` and `Inline` #95398

Add query to check for impossible predicates, use it to skip `NoopMethodCall` and `Inline` #95398