Allow to feed a value in another query's cache and remove `WithOptConstParam` #96840

cjgillot · 2022-05-08T15:30:58Z

I used it to remove WithOptConstParam queries, as an example.

The idea is that a query (here typeck(function)) can write into another query's cache (here type_of(anon const)). The dependency node for type_of would depend on all the current dependencies of typeck.

There is still an issue with cycles: if type_of(anon const) is accessed before typeck(function), we will still have the usual cycle. The way around this issue is to ensure that typeck(function) is called before accessing type_of(anon const).

When replayed, we may the following cases:

typeck is green, in that case type_of is green too, and all is right;
type_of is green, typeck may still be marked as red (it depends on strictly more things than type_of) -> we verify that the saved value and the re-computed value of type_of have the same hash;
type_of is red, then typeck is red -> it's the caller responsibility to ensure typeck is recomputed before type_of.

As anon consts have their own DefPathData, it's not possible to have the def-id of the anon-const point to something outside the original function, but the general case may have to be resolved before using this device more broadly.

There is an open question about loading from the on-disk cache. If typeck is loaded from the on-disk cache, the side-effect does not happen. The regular type_of implementation can go and fetch the correct value from the decoded typeck results, and the dep-graph will check that the hashes match, but I'm not sure we want to rely on this behaviour.

I specifically allowed to feed the value to type_of from inside a call to type_of. In that case, the dep-graph will check that the fingerprints of both values match.

This implementation is still very sensitive to cycles, and requires that we call typeck(function) before typeck(anon const). The reason is that typeck(anon const) calls type_of(anon const), which calls typeck(function), which feeds type_of(anon const), and needs to build the MIR so needs typeck(anon const). The latter call would not cycle, since type_of(anon const) has been set, but I'd rather not remove the cycle check.

bjorn3 · 2022-05-08T16:14:19Z

compiler/rustc_middle/src/ty/query.rs

+        impl<$tcx> TyCtxtFeed<$tcx> {
+            $($(#[$attr])*
+            #[inline(always)]
+            pub fn $name<V>(


Would it make sense to only allow this for queries which explicitly opt-in? And then maybe not allow setting any provider for such queries. I think that makes the information flow a lot clearer.

bors · 2022-05-09T22:59:49Z

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

rustbot · 2022-09-04T17:12:38Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in const_evaluatable.rs

cc @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

cjgillot

Thanks rustbot. So much for the discretion of a WIP PR 😄

compiler/rustc_query_system/src/dep_graph/graph.rs

cjgillot · 2022-09-04T22:12:01Z

r? compiler

bors · 2022-09-05T05:05:40Z

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

lcnr · 2022-09-05T05:10:26Z

looked through this PR and I am really happy about not seeing WithOptConstParam anymore ✨

The query system is complex enough for me to not be comfortable approving this. My thoughts:

we should probably change the type_of query to ICE if opt_const_param_of returns Some. Or actually, we should remove the opt_const_param_of query entirely and put the branches from there into type_of and ICE for branches which would result in Some.
doing that should change the buggy behavior when not using feed from query cycle to ICE (with a good message). So I am not that concerned about the impact on other rustc devs using type_of, especially compared to understanding WithOptConstParam which is a lot more involved.
being feedable should be opt-in, as mentioned by @bjorn3. We should probably even change it so that queries are either feedable or provided. For that we would have to split type_of into type_of and anon_const_type or whatever 🤔
if we have type_of calling another query which then feeds the value of type_of (which is currently getting computed), we should ICE I think? I don't think this is currently covered, but haven't looked into the query impl too much ^^

going to hand off this review to incr comp people, but I am looking forward to this PR landing. I do think it requires an MCP though.

r? @wesleywiser maybe, @michaelwoerister isn't available much rn afaict

cjgillot · 2022-09-05T09:17:01Z

* we should probably change the `type_of` query to ICE if `opt_const_param_of` returns `Some`. Or actually, we should remove the `opt_const_param_of` query entirely and put the branches from there into `type_of` and ICE for branches which would result in `Some`.

This is not really possible yet. I could remove the case where tcx.typeck was invoked, but other uses remain. For instance, types in signatures cannot be dealt with that way, because const_evaluatable_predicates_of needs them.

* doing that should change the buggy behavior when not using `feed` from query cycle to ICE (with a good message). So I am not that concerned about the impact on other rustc devs using `type_of`, especially compared to understanding `WithOptConstParam` which is a lot more involved.

Mmh... I wonder where to put this message. Do you have an example of such misuse?

* being feedable should be opt-in, as mentioned by @bjorn3. We should probably even change it so that queries are either feedable or provided. For that we would have to split `type_of` into `type_of` and `anon_const_type` or whatever thinking

Done.

* if we have `type_of` calling another query which then feeds the value of `type_of` (which is currently getting computed), we should ICE I think? I don't think this is currently covered, but haven't looked into the query impl too much ^^

We do not ICE, we check that both values' fingerprints match.

cjgillot · 2022-09-05T10:05:18Z

@bors try @rust-timer queue

rust-timer · 2022-09-05T10:05:20Z

Awaiting bors try build completion.

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

cjgillot · 2023-04-21T06:22:48Z

@bors r=oli-obk

bors · 2023-04-21T06:22:50Z

📌 Commit 9bab866 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-04-21T08:05:02Z

⌛ Testing commit 9bab866 with merge 1f5768b...

bors · 2023-04-21T10:22:15Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1f5768b to master...

bors · 2023-04-21T10:22:15Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1f5768b to master...

nbdd0121 · 2023-04-21T10:36:46Z

rust/compiler/rustc_middle/src/ty/consts.rs

Line 129 in 1f5768b

// not the expected parameter type from WithOptConstParam.

One reference to WithOptConstParam remaining in the comment

rust-timer · 2023-04-21T11:38:38Z

Finished benchmarking commit (1f5768b): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-0.8%	[-2.1%, -0.3%]	57
Improvements ✅ (secondary)	-0.9%	[-1.7%, -0.3%]	26
All ❌✅ (primary)	-0.8%	[-2.1%, -0.3%]	57

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	4.4%	[1.4%, 9.3%]	4
Improvements ✅ (primary)	-5.7%	[-12.0%, -0.4%]	8
Improvements ✅ (secondary)	-3.3%	[-4.4%, -2.2%]	6
All ❌✅ (primary)	-5.0%	[-12.0%, 0.4%]	9

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.3%, 2.7%]	2
Improvements ✅ (primary)	-2.4%	[-5.1%, -1.0%]	14
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.2%]	2
All ❌✅ (primary)	-2.4%	[-5.1%, -1.0%]	14

Add regression tests for const-generic inherent associated types Fixes rust-lang#109759. The tests are no longer failing since rust-lang#96840 which was merged recently (rust-lang#109410 is no longer necessary for them). `@rustbot` label F-inherent_associated_types

Allow to feed a value in another query's cache and remove `WithOptConstParam` I used it to remove `WithOptConstParam` queries, as an example. The idea is that a query (here `typeck(function)`) can write into another query's cache (here `type_of(anon const)`). The dependency node for `type_of` would depend on all the current dependencies of `typeck`. There is still an issue with cycles: if `type_of(anon const)` is accessed before `typeck(function)`, we will still have the usual cycle. The way around this issue is to `ensure` that `typeck(function)` is called before accessing `type_of(anon const)`. When replayed, we may the following cases: - `typeck` is green, in that case `type_of` is green too, and all is right; - `type_of` is green, `typeck` may still be marked as red (it depends on strictly more things than `type_of`) -> we verify that the saved value and the re-computed value of `type_of` have the same hash; - `type_of` is red, then `typeck` is red -> it's the caller responsibility to ensure `typeck` is recomputed *before* `type_of`. As `anon consts` have their own `DefPathData`, it's not possible to have the def-id of the anon-const point to something outside the original function, but the general case may have to be resolved before using this device more broadly. There is an open question about loading from the on-disk cache. If `typeck` is loaded from the on-disk cache, the side-effect does not happen. The regular `type_of` implementation can go and fetch the correct value from the decoded `typeck` results, and the dep-graph will check that the hashes match, but I'm not sure we want to rely on this behaviour. I specifically allowed to feed the value to `type_of` from inside a call to `type_of`. In that case, the dep-graph will check that the fingerprints of both values match. This implementation is still very sensitive to cycles, and requires that we call `typeck(function)` before `typeck(anon const)`. The reason is that `typeck(anon const)` calls `type_of(anon const)`, which calls `typeck(function)`, which feeds `type_of(anon const)`, and needs to build the MIR so needs `typeck(anon const)`. The latter call would not cycle, since `type_of(anon const)` has been set, but I'd rather not remove the cycle check.

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

This comment has been minimized.

Sign in to view

bjorn3 reviewed May 8, 2022

View reviewed changes

cjgillot changed the title ~~WIP: Allow to feed a value in another's cache~~ WIP: Allow to feed a value in another query's cache May 8, 2022

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 9, 2022

cjgillot closed this Jul 30, 2022

cjgillot reopened this Sep 4, 2022

cjgillot force-pushed the query-feed branch from b2ab513 to 663391f Compare September 4, 2022 17:12

rustbot added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Sep 4, 2022

This comment has been minimized.

Sign in to view

cjgillot force-pushed the query-feed branch from 663391f to c00b835 Compare September 4, 2022 17:18

This comment has been minimized.

Sign in to view

cjgillot commented Sep 4, 2022

View reviewed changes

compiler/rustc_query_system/src/dep_graph/graph.rs Outdated Show resolved Hide resolved

cjgillot changed the title ~~WIP: Allow to feed a value in another query's cache~~ Allow to feed a value in another query's cache Sep 4, 2022

rust-highfive assigned lcnr Sep 4, 2022

rust-highfive assigned wesleywiser and unassigned lcnr Sep 5, 2022

cjgillot force-pushed the query-feed branch from 6da246f to c9439e7 Compare September 5, 2022 08:45

cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2022

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

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 20, 2023

Bless coverage.

9bab866

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2023

bors merged commit 1f5768b into rust-lang:master Apr 21, 2023

rustbot added this to the 1.71.0 milestone Apr 21, 2023

bors mentioned this pull request Apr 21, 2023

Perform unused assignment and unused variables lints on MIR. #101500

Open

cjgillot deleted the query-feed branch April 21, 2023 11:02

fmease mentioned this pull request Apr 25, 2023

Add regression tests for const-generic inherent associated types #110817

Merged

matthiaskrgr mentioned this pull request Apr 28, 2023

ICE: Trying to feed an already recorded value for query type_of key #110887

Closed

zhassan-aws mentioned this pull request May 18, 2023

Update rust toolchain to 2023-04-29 model-checking/kani#2452

Merged

4 tasks

apiraino mentioned this pull request Oct 11, 2023

format_args! does not respect integer literals' type #115423

Closed

oli-obk mentioned this pull request Mar 5, 2024

Remove feed_local_def_id #121089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to feed a value in another query's cache and remove `WithOptConstParam` #96840

Allow to feed a value in another query's cache and remove `WithOptConstParam` #96840

cjgillot commented May 8, 2022 •

edited

Loading

This comment has been minimized.

bjorn3 May 8, 2022

bors commented May 9, 2022

rustbot commented Sep 4, 2022

This comment has been minimized.

This comment has been minimized.

cjgillot left a comment

cjgillot commented Sep 4, 2022

bors commented Sep 5, 2022

lcnr commented Sep 5, 2022

cjgillot commented Sep 5, 2022 •

edited

Loading

cjgillot commented Sep 5, 2022

rust-timer commented Sep 5, 2022

cjgillot commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

nbdd0121 commented Apr 21, 2023

rust-timer commented Apr 21, 2023

Allow to feed a value in another query's cache and remove WithOptConstParam #96840

Allow to feed a value in another query's cache and remove WithOptConstParam #96840

Conversation

cjgillot commented May 8, 2022 • edited Loading

This comment has been minimized.

bjorn3 May 8, 2022

Choose a reason for hiding this comment

bors commented May 9, 2022

rustbot commented Sep 4, 2022

This comment has been minimized.

This comment has been minimized.

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot commented Sep 4, 2022

bors commented Sep 5, 2022

lcnr commented Sep 5, 2022

cjgillot commented Sep 5, 2022 • edited Loading

cjgillot commented Sep 5, 2022

rust-timer commented Sep 5, 2022

cjgillot commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

nbdd0121 commented Apr 21, 2023

rust-timer commented Apr 21, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Allow to feed a value in another query's cache and remove `WithOptConstParam` #96840

Allow to feed a value in another query's cache and remove `WithOptConstParam` #96840

cjgillot commented May 8, 2022 •

edited

Loading

cjgillot commented Sep 5, 2022 •

edited

Loading