Suppress errors in const eval during trait selection v2 #92674

b-naber · 2022-01-08T16:58:59Z

This is the second attempt of #92653

Significantly reduces the complexity of the previous PR. Hopefully also result in better perf results.

Needs a perf-run.

r? @oli-obk

cc @lcnr @RalfJung

Co-authored-by: bjorn3 <[email protected]>

rust-highfive · 2022-01-08T16:59:02Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

b-naber · 2022-01-08T17:26:34Z

Don't understand why CI outputs that fmt failure. I did run tidy with --bless locally.

oli-obk · 2022-01-08T19:35:38Z

@bors try @rust-timer queue

rust-timer · 2022-01-08T19:35:40Z

Awaiting bors try build completion.

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

bors · 2022-01-08T19:35:47Z

⌛ Trying commit 53c0bab with merge fcb93cd944d39604d20b8ea2ec20a15d08c2f84b...

bors · 2022-01-08T21:26:16Z

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

rust-timer · 2022-01-08T21:26:17Z

Queued fcb93cd944d39604d20b8ea2ec20a15d08c2f84b with parent 488acf8, future comparison URL.

rust-timer · 2022-01-08T22:56:04Z

Finished benchmarking commit (fcb93cd944d39604d20b8ea2ec20a15d08c2f84b): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Small improvement in instruction counts (up to -1.1% on incr-unchanged builds of unicode_normalization)
Very large regression in instruction counts (up to 12.1% on incr-full builds of issue-46449)

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.

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

oli-obk · 2022-01-11T10:17:09Z

The notable thing in the perf regression is that eval_to_allocation_raw is now called in issue-46449 check | incr-unchanged, when it wasn't called at all before.

oli-obk · 2022-01-11T10:27:35Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+        let dedup_result = alloc_map.get(&id);
+        debug!(?dedup_result);
+
+        match param_env.reveal() {


these matches are not obvious to me. Can you explain them? I'm unsure what the rules are here.

The rationale for those was as follows:

If we have stored a successful allocation with Reveal::Selection then we can deduplicate that result on calls of dedup_eval_alloc_raw with both Reveal::Selection and Reveal::UserFacing (since Selection and UserFacing only differ in that we keep certain errors silent with Selection, but successful allocations are always equivalent). This is also the reason why we can deduplicate from saved calls with UserFacingon calls with both Selection and UserFacing.

If we try to deduplicate with Reveal::All we should be able to deduplicate from saved query results with both Selection and UserFacing, since All will always succeed if either UserFacing or Selection have succeeded. On the other hand if we have a saved result with Reveal::All, it's not guaranteed that query calls with Reveal::Selection or Reveal::UserFacing succeed, so we can't deduplicate from those.

👍 please copy this comment into the source, it explains this perfectly

oli-obk · 2022-01-11T14:34:22Z

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

+                    }
+                    _ => {}
+                }
+                tcx.sess.struct_span_err(span, "failed to evaluate the given constant")


all cases of this in the test suite are redundant.

EDIT: ah no, I just grepped badly...

This error message is still often redundant and then just noise. Not sure how to improve it though...

b-naber · 2022-01-11T14:42:19Z

The notable thing in the perf regression is that eval_to_allocation_raw is now called in issue-46449 check | incr-unchanged, when it wasn't called at all before.

Yeah that's weird. issue-46446 also ICEs with a fingerprint error. I'll try to investigate.

b-naber · 2022-01-21T10:18:11Z

The calls of eval_to_allocation_raw in the incr-unchanged scenario should be fixed now, i.e. they shouldn't occur anymore (at least I don't get those calls locally anymore).

@oli-obk Can you request another perf-run please?

b-naber · 2022-01-21T10:19:20Z

I'll address the review comments after a successful perf-run.

oli-obk · 2022-01-21T10:23:25Z

@bors try @rust-timer queue

rust-timer · 2022-01-21T10:23:27Z

Awaiting bors try build completion.

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

bors · 2022-01-21T10:23:36Z

⌛ Trying commit 78e4b6f with merge 74db553a3539cb722fc57e34b88a6871c7a89933...

bors · 2022-01-21T12:14:01Z

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

rust-timer · 2022-01-21T12:14:03Z

Queued 74db553a3539cb722fc57e34b88a6871c7a89933 with parent 0bcacb3, future comparison URL.

rust-timer · 2022-01-21T13:55:06Z

Finished benchmarking commit (74db553a3539cb722fc57e34b88a6871c7a89933): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

Small improvement in instruction counts (up to -1.2% on incr-full builds of ctfe-stress-4 opt)

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

rust-log-analyzer · 2022-01-24T11:49:59Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
-                            span_bug!(
-                                constant.span,
-                                "codegen encountered silent error",
-                            );
+                            span_bug!(constant.span, "codegen encountered silent error",);
                     }
                 }
                 }
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_codegen_cranelift/build_system/rustc_info.rs" "/checkout/compiler/rustc_codegen_cranelift/build_sysroot/src/lib.rs" "/checkout/compiler/rustc_codegen_cranelift/src/cast.rs" "/checkout/compiler/rustc_codegen_cranelift/src/constant.rs" "/checkout/compiler/rustc_codegen_cranelift/src/debuginfo/unwind.rs" "/checkout/compiler/rustc_codegen_cranelift/src/common.rs" "/checkout/compiler/rustc_codegen_cranelift/src/debuginfo/object.rs" "/checkout/compiler/rustc_codegen_cranelift/build_system/build_sysroot.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

lcnr · 2022-01-25T10:25:21Z

Your approach sadly doesn't work, adding interior mutability to the TyCtxt is pretty much always unsound wrt incremental compilation.

For an example how your current approach could break, consider the following scenario:

initial compilation
- in queryA (e.g. typeck) we evaluate a constant C using dedup_eval_alloc_raw which calls the query eval_to_allocation_raw for C with Reveal::UserFacing. This adds a dependency edge for queryA to eval_to_allocation_raw.
- in queryB (e.g. collect_and_partition_mono_items) we again evaluate a constant using dedup_eval_alloc_raw with Reveal::All. dedup_eval_alloc_raw reuses the result of the call in typeck, meaning that we don't call eval_to_allocation_raw in queryB. There is therefore no dependency edge from queryB to eval_to_allocation_raw
recompilation with changes to body of constant B
- we check whether queryA has to be recomputed, see that eval_to_allocation_raw for C depends on things which changed, and therefore recompute eval_to_allocation_raw for queryA to check whether it is still the same. Let's assume that the new return value is different. We rerun queryA using the new result from eval_to_allocation_raw and everything is ok
- we check whether queryB has to be recomputed. All queries queryB depends on did not change their output, therefore we don't have to recompute queryB which causes subtle bugs and unsoundness. The issue here is that queryB has to check that eval_to_allocation_raw has changed since the last compilation, but because we avoided the query system by using interior mutability, this is not the case.

I believe that the only way to soundly avoid recomputations with different Reveals are the approaches from #81339 and something like #92653 (haven't looked too deeply into that PR). I personally would like to something like #92653 because that also allows us to add much better spans to const eval errors in general, consider something like this. We should be able to return the eval errors from queries without a significant perf impact, and can probably design an api which makes any misuse (e.g. by forgetting to emit eval errors) very difficult. Am definitely open to talking about this in sync if you're interested.

Sorry for only looking at this PR now and thanks for working on this 🙇❤️

b-naber · 2022-01-25T16:38:45Z

@lcnr Thanks for the review.

I think the problem you describe should be solved by the changes introduced in the commit simplify deduplication and better utilize query caching. Note that we always call the query after these changes and make use of the result cached by the query system. So there should be a dependency edge for queryB to eval_to_allocation_raw. I could be wrong but, prior to this change we also got an ICE in the query system on the benchmark issue-46446, which we don't get anymore after this commit. Have you looked at that commit?

lcnr · 2022-01-25T17:09:40Z

yes, my example was incorrect after this commit, but the issue that queries should remain pure functions still remains. We now get the following scenario:

initial compilation
- in queryA (e.g. typeck) we evaluate a constant C using dedup_eval_alloc_raw which calls the query eval_to_allocation_raw for C with Reveal::UserFacing. This adds a dependency edge for queryA to eval_to_allocation_raw.
- in queryB (e.g. collect_and_partition_mono_items) we again evaluate a constant using dedup_eval_alloc_raw with Reveal::All. dedup_eval_alloc_raw sees that eval_to_allocation_raw with Reveal::UserFacing already succeeded, so we now call that. There is therefore no dependency edge from queryB to eval_to_allocation_raw with Reveal::All. (edit: previously wrote Reveal::UserFacing here)
recompilation with changes to body of constant C
- we check whether queryB has to be recomputed. As it depends on eval_to_allocation_raw with Reveal::UserFacing, it gets recomputed and now calls eval_to_allocation_raw with Reveal::All, as no previous query called eval_to_allocation_raw with Reveal::UserFacing.

This might seem fine as eval_to_allocation_raw with Reveal::UserFacing is only used in queryB if it's output is the same as with Reveal::All, so we never incorrectly mark queryB as green.

It still has issues, most noticeably this introduces nondeterminism in the compiler (especially once we run queries in parallel, which we do want). If we first compute queryB and then queryA then eval_to_allocation_raw with Reveal::All is called, otherwise not. This can duplicate errors and trigger internal soundness checks - either existing ones or ones added in the future.

I wouldn't be surprised if this causes other subtle issues for our query system, at this point you have to ask people more involved than me

b-naber · 2022-01-25T17:53:08Z

Ok, thanks for clarifying this. I can close this PR, but I would definitely still be interested in working on the issue and getting the idea implemented.

There is one thing I don't understand about your example though:

QueryB depends on eval_to_allocation_raw with Reveal::All, but we never call eval_to_allocation_raw(Reveal::All) directly from queryB, as this goes through dedup_eval_alloc.dedup_eval_allocnow calls eval_to_allocation_raw(Reveal::UserFacing), so queryB effecively depends on eval_to_allocation(Reveal::UserFacing) now, so why does this not cause a dependency edge queryB -> eval_to_allocation_raw(Reveal::UserFacing) to be created?

It can't be because the result is already cached, because in a scenario in which we didn't have the deduplication logic and in which some other queryC had already computed eval_to_allocation_raw(Reveal::All), we would try to call eval_to_alloc(Reveal::All) from queryB, get the cached result, but still create an edge queryB -> eval_alloc(Reveal::All) in the dependency graph. So why wouldn't we do the analogous thing with the deduplication logic, where queryB really depends on eval_to_alloc(Reveal::UserFacing)?

lcnr · 2022-01-25T19:22:25Z

that was me not being careful enough while writing it, should have fixed it in the above comment. queryB will have an edge to eval_to_allocation_raw(Reveal::UserFacing) but not to eval_to_allocation_raw(Reveal::All). I hope the explanation makes more sense now.

Ok, thanks for clarifying this. I can close this PR, but I would definitely still be interested in working on the issue and getting the idea implemented.

definitely. I would be interested in you reviving #92653. I should be able to take the time for a first look at it this week, either after you're reopened it or even before then if it is already somewhat ready (ignoring perf for now)

b-naber · 2022-01-25T19:42:09Z

Thanks, that makes sense to me now. I'll ping you about further work on this issue on zulip.

lcnr and others added 9 commits December 28, 2021 11:04

add new Reveal for const eval during selection

3c78e9e

update tests

10e5f1a

typo

bbd8ef9

Co-authored-by: bjorn3 <[email protected]>

rebase

80955cd

introduce Reveal::Selection and deduplication for eval const queries

8cb36a0

update queries for deduplication

773101f

update NotConstEvaluatable

8acef87

update trait selection

74b2918

update tests

53c0bab

rust-highfive assigned oli-obk Jan 8, 2022

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

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

b-naber mentioned this pull request Jan 8, 2022

Suppress errors in const eval during trait selection #92653

Closed

This comment has been minimized.

Sign in to view

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

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

oli-obk reviewed Jan 11, 2022

View reviewed changes

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

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 21, 2022

lcnr self-assigned this Jan 21, 2022

This comment has been minimized.

Sign in to view

b-naber force-pushed the 2nd-suppress-const-eval-errs branch from 864a3e8 to 6ead573 Compare January 21, 2022 15:36

This comment has been minimized.

Sign in to view

address review

66a689c

b-naber force-pushed the 2nd-suppress-const-eval-errs branch from 6ead573 to 66a689c Compare January 24, 2022 11:38

lcnr 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 Jan 25, 2022

b-naber closed this Jan 25, 2022

oli-obk mentioned this pull request Mar 25, 2024

Evaluating constants in MIR optimizations introduces const eval errors #122828

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress errors in const eval during trait selection v2 #92674

Suppress errors in const eval during trait selection v2 #92674

b-naber commented Jan 8, 2022

rust-highfive commented Jan 8, 2022

This comment has been minimized.

b-naber commented Jan 8, 2022

oli-obk commented Jan 8, 2022

rust-timer commented Jan 8, 2022

bors commented Jan 8, 2022

bors commented Jan 8, 2022

rust-timer commented Jan 8, 2022

rust-timer commented Jan 8, 2022

oli-obk commented Jan 11, 2022

oli-obk Jan 11, 2022

b-naber Jan 11, 2022

oli-obk Jan 17, 2022

oli-obk Jan 11, 2022 •

edited

Loading

b-naber commented Jan 11, 2022 •

edited

Loading

b-naber commented Jan 21, 2022 •

edited

Loading

b-naber commented Jan 21, 2022

oli-obk commented Jan 21, 2022

rust-timer commented Jan 21, 2022

bors commented Jan 21, 2022

This comment has been minimized.

bors commented Jan 21, 2022

rust-timer commented Jan 21, 2022

rust-timer commented Jan 21, 2022

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Jan 24, 2022

lcnr commented Jan 25, 2022

b-naber commented Jan 25, 2022

lcnr commented Jan 25, 2022 •

edited

Loading

b-naber commented Jan 25, 2022

lcnr commented Jan 25, 2022

b-naber commented Jan 25, 2022

Suppress errors in const eval during trait selection v2 #92674

Suppress errors in const eval during trait selection v2 #92674

Conversation

b-naber commented Jan 8, 2022

rust-highfive commented Jan 8, 2022

This comment has been minimized.

b-naber commented Jan 8, 2022

oli-obk commented Jan 8, 2022

rust-timer commented Jan 8, 2022

bors commented Jan 8, 2022

bors commented Jan 8, 2022

rust-timer commented Jan 8, 2022

rust-timer commented Jan 8, 2022

oli-obk commented Jan 11, 2022

oli-obk Jan 11, 2022

Choose a reason for hiding this comment

b-naber Jan 11, 2022

Choose a reason for hiding this comment

oli-obk Jan 17, 2022

Choose a reason for hiding this comment

oli-obk Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

b-naber commented Jan 11, 2022 • edited Loading

b-naber commented Jan 21, 2022 • edited Loading

b-naber commented Jan 21, 2022

oli-obk commented Jan 21, 2022

rust-timer commented Jan 21, 2022

bors commented Jan 21, 2022

This comment has been minimized.

bors commented Jan 21, 2022

rust-timer commented Jan 21, 2022

rust-timer commented Jan 21, 2022

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Jan 24, 2022

lcnr commented Jan 25, 2022

b-naber commented Jan 25, 2022

lcnr commented Jan 25, 2022 • edited Loading

b-naber commented Jan 25, 2022

lcnr commented Jan 25, 2022

b-naber commented Jan 25, 2022

oli-obk Jan 11, 2022 •

edited

Loading

b-naber commented Jan 11, 2022 •

edited

Loading

b-naber commented Jan 21, 2022 •

edited

Loading

lcnr commented Jan 25, 2022 •

edited

Loading