Rejig `rustc_with_all_queries!`
There are three things relating to `rustc_with_all_queries!` that have been bugging me.
- `rustc_with_all_queries!`'s ability to receive `extra_fake_queries` lines like `[] fn Null(()) -> (),` where the only real thing is the `Null`, and everything is just pretending to be a normal query, ugh.
- `make_dep_kind_array!`: a macro produced by one macro (`define_dep_nodes!`) and used by another macro (`define_queries!`) in another crate, ugh.
- The `_dep_kind_vtable_ctors` module, which is a special module with no actual code that serves just a way of collecting vtable constructors from two different places so they can be referred to by `make_dep_kind_array!`, ugh.
By making some adjustments to how `rustc_with_all_queries!` works, all three of these things are eliminated.
r? @Zalathar
Currently `define_dep_nodes` produces a macro `make_dep_kind_array` that
encodes the names of non-queries followed by queries. This macro is used
by `make_dep_kind_vtables` to make the full array of vtables, by
referring to vtable constructor functions that are put into `mod
_dep_kind_vtable_ctors`. Pretty weird!
This commit takes advantage of the previous commit's changes to
`rustc_with_all_queries`, which makes both query and non-query
information available. A new call to `rustc_with_all_queries` is used to
construct the vtable array. (This moves some dep_kind_vtable code from
`plumbing.rs` to `dep_kind_vtables.rs`, which is good.) It's
straightforward now with iterator chaining, and `mod
_dep_kind_vtable_ctors` is no longer needed.
`rustc_with_all_queries` currently provides information about all
queries. Also, a caller can provide an ad hoc list of extra non-queries.
This is used by `define_queries` for non-query dep kinds: `Null`, `Red`,
etc. This is pretty hacky.
This commit changes `rustc_with_all_queries` so that the non-queries
information is available to all callers. (Some callers ignore the
non-query information.) This is done by adding `non_query` entries to
the primary list of queries in `rustc_queries!`.
It has no effect.
`symbol_name` is the only query that produces a `SymbolName`. If it was
marked with `cycle_delayed_bug`/`cycle_stash` then this `FromCycleError`
impl would make sense, but that's not the case. Maybe it was the case in
the past.
`Value` is an unhelpfully generic name. Standard naming procedure for a
trait with a single method is for the trait name to match the method
name, which is what this commit does. Likewise, the enclosing module is
renamed from `values` to `from_cycle_error`.
Also add a comment about some non-obvious behaviour.
Improve the forcing/promotion functions in `DepKindVTable`
This is a bundle of changes to the two function pointers in `DepKindVTable` that are responsible for forcing dep nodes, or promoting disk-cached values from the previous session into memory.
The perf improvements to incr-unchanged and incr-patched are likely from skipping more of the “promotion” plumbing for queries that never cache to disk.
- `force_from_dep_node` → `force_from_dep_node_fn`
- `try_load_from_disk_cache` → `promote_from_disk_fn`
This commit also inlines the wrapper function around `promote_from_disk_fn`.
Clean up `QueryVTable::hash_result` into `hash_value_fn`
This PR:
- Renames the query vtable field `hash_result` to `hash_value_fn`
- Removes the unhelpful `HashResult` type alias, which was hiding an important layer of `Option`
- Replaces the cryptic `hash_result!` helper macro with a more straightforward `if_no_hash!` helper, in line with other modifier-checking macros
- Renames a few identifiers to refer to a query's return value as `value`
There should be no change to compiler behaviour.
r? nnethercote (or compiler)
Remove query function arrays
`define_queries!` produces four arrays of function pointers, which other functions iterate over. These aren't actually necessary.
r? @petrochenkov
This commit:
- Renames the query vtable field `hash_result` to `hash_value_fn`
- Removes the unhelpful `HashResult` type alias, which was hiding an
important layer of `Option`
- Replaces the cryptic `hash_result!` helper macro with a more straightforward
`if_no_hash!` helper, in line with other modifier-checking macros
- Renames a few identifiers to refer to a query's return value as `value`
Don't allow subdiagnostic to use variables from their parent
Tangentially related to https://github.com/rust-lang/rust/issues/151366
This is PR 1/2 for structured diagnostics, will do the unstructured ones next. After the second PR I will be able to remove some code that should compensate for this PR being positive.
Regardless of this PR having a positive diff, I feel that subdiagnostics being able to use variables from their parent is very confusing, so this is for the better,.
r? @jdonszelmann
And also `alloc_self_profile_query_strings` for each query. This is done
by generating the top-level `alloc_self_profile_query_strings` and
having it do things more directly.
Currently `gather_active_jobs` and `gather_active_jobs_inner` do some of
the work each. This commit changes things so that `gather_active_jobs`
is just a thin wrapper around `gather_active_jobs_inner`. This paves the
way for removing `gather_active_jobs` in the next commit.
Clean up some code related to `QueryVTable::execute_query_fn`
This PR is an assortment of small cleanups to code that interacts with `execute_query_fn` in the query vtable.
I also experimented with trying to replace the macro-generated `__rust_end_short_backtrace` functions with a single shared generic function, but I couldn't manage to avoid breaking short backtraces, so I left a note behind to document my attempt.
r? nnethercote (or compiler)
`QuerySystem` has two function pointers: `encode_query_results` and
`try_mark_green`. These exist so that `rustc_middle` can call functions
from upstream crates.
But we have a more general mechanism for that: hooks. So this commit
converts these two cases into hooks.
`QueryEngine` is a struct with one function pointer per query. This
commit removes it by moving each function pointer into the relevant
query's vtable, which is already a collection of function pointers for
that query. This makes things simpler.
Currently type variables that impl `QueryCache` are called either `C` or
`Cache`. I find the former clearer because single char type variables
names are very common and longer type variable names are easy to mistake
for type or trait names -- e.g. I find the trait bound `C: QueryCache`
much easier and faster to read than `Cache: QueryCache`.
`QuerySystem` has these fields:
```
pub states: QueryStates<'tcx>,
pub caches: QueryCaches<'tcx>,
pub query_vtables: PerQueryVTables<'tcx>,
```
Each one has an entry for each query.
Some methods that take a query-specific `QueryVTable` need to access the
corresponding query-specific states and/or caches. So `QueryVTable`
stores the *byte offset* to the relevant fields within `states` and
`caches`, and uses that to (with `unsafe`) access the fields. This is
bizarre, and I hope it made sense in the past when the code was
structured differently.
This commit moves `QueryState` and `QueryCache` inside `QueryVTable`. As
a result, the query-specific states and/or caches are directly
accessible, and no unsafe offset computations are required. Much simpler
and more normal. Also, `QueryCaches` and `QueryStates` are no longer
needed, which makes `define_callbacks!` a bit simpler.
The commit also rename the fields and their getters to `states` and
`caches`.
Clarify how "ensure" queries check whether they can skip execution
The current `check_cache: bool` field in QueryMode is confusing for a few reasons:
- It actually refers to checking the *disk cache*, not the in-memory cache.
- It gives no indication of what condition is being checked, or why.
- It obscures the link between `tcx.ensure_ok()` and `tcx.ensure_done()`, and the actual check.
This PR replaces that field with an `EnsureMode` enum that distinguishes between ensure-ok and ensure-done, and leaves the actual check as an implementation detail of the ensure-done mode.
This PR also renames `ensure_must_run` → `check_if_ensure_can_skip_execution`, and uses a return struct to more clearly indicate how this helper function gives permission for its caller to skip execution of a query that would otherwise be executed. This also inverts the sense of the returned boolean, which was previously “must run” but is now ”skip execution”.
r? nnethercote (or compiler)
It's now `query_vtable` because its return type changed. And thanks to
the previous commit it can be manually inlined in several places. (The
only remaining calls to it are in `make_dep_kind_vtable_for_query`,
which are more challenging to remove.)
`SemiDynamicQueryDispatcher` is just a `QueryVTable` wrapper with an
additional `const FLAGS: QueryFlags` generic parameter that contains
three booleans. This arrangement exists as a performance optimization.
But the performance effects are very small and it adds quite a bit of
complexity to an already overly-complex part of the codebase. If it
didn't exist and somebody proposed adding it and asked me to review, I
almost certainly wouldn't approve it.
This commit removes it. The three booleans in `QueryFlags` are moved
into `QueryVTable` The non-trivial methods of
`SemiDynamicQueryDispatcher` become methods of `QueryVTable`.
Within `define_callbacks!`:
- Use `Key<'tcx>` where possible instead of `$($K)*`.
- Use `Value<'tcx>` where possible instead of `$V`.
In the other `define_*!` macros:
- Use `$K:ty` where possible instead of `$($K:tt)*`.
- Add comments about unused `$K` and `$V` cases.
- Add `()` key types to the special nodes in the `define_dep_nodes!`
invocation for consistency with normal queries.
There is a bunch of plumbing to record the string `"foo"` for each
variant `DepKind::foo`. But that's what the `Debug `impl` now produces.
So this commit removes the plumbing.
It was removed in #115920 to enable it being moved to
`rustc_query_system`, a move that has recently been reversed. It's much
simpler as an enum.
Also:
- Remove the overly complicated `Debug` impl for `DepKind`.
- Remove the trivial `DepKind` associated constants (`NULL` et al.)
- Add an assertion to ensure that the number of `DepKinds` fits within a
`u16`.
- Rename `DEP_KIND_VARIANTS` as `DEP_KIND_NUM_VARIANTS`, to make it
clearer that it's a count, not a collection.
- Use `stringify!` in one place to make the code clearer.
Jonathan Brouwer