coverage: Prepare for upcoming changes to counter creation #135873
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zalathar
added
the
A-code-coverage
|
rustbot has assigned @matthewjasper. Use |
rustbot
added
S-waiting-on-review
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 23, 2025
Allow `arena_cache` queries to return `Option<&'tcx T>` Currently, `arena_cache` queries always have to return `&'tcx T`[^deref]. This means that if an arena-cached query wants to return an optional value, it has to return `&'tcx Option<T>`, which has a few negative consequences: - It goes against normal Rust style, where `Option<&T>` is preferred over `&Option<T>`. - Callers that actually want an `Option<&T>` have to manually call `.as_ref()` on the query result. - When the query result is `None`, a full-sized `Option<T>` still needs to be stored in the arena. This PR solves that problem by introducing a helper trait `ArenaCached` that is implemented for both `&T` and `Option<&T>`, and takes care of bridging between the provided type, the arena-allocated type, and the declared query return type. --- To demonstrate that this works, I have converted the two existing arena-cached queries that currently return `&Option<T>`: `mir_coroutine_witnesses` and `diagnostic_hir_wf_check`. Only the query declarations need to be modified; existing providers and callers continue to work with the new query return type. (My real goal is to apply this to `coverage_ids_info`, which will return Option as of rust-lang#135873, but that PR hasn't landed yet.) [^deref]: Technically they could return other types that implement `Deref`, but it's hard to imagine this working well with anything other than `&T`.
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt This PR changes MIR |
|
Rebased and fixed a trivial typo. |
oli-obk
approved these changes
Jan 23, 2025
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 23, 2025
…piler-errors Allow `arena_cache` queries to return `Option<&'tcx T>` Currently, `arena_cache` queries always have to return `&'tcx T`[^deref]. This means that if an arena-cached query wants to return an optional value, it has to return `&'tcx Option<T>`, which has a few negative consequences: - It goes against normal Rust style, where `Option<&T>` is preferred over `&Option<T>`. - Callers that actually want an `Option<&T>` have to manually call `.as_ref()` on the query result. - When the query result is `None`, a full-sized `Option<T>` still needs to be stored in the arena. This PR solves that problem by introducing a helper trait `ArenaCached` that is implemented for both `&T` and `Option<&T>`, and takes care of bridging between the provided type, the arena-allocated type, and the declared query return type. --- To demonstrate that this works, I have converted the two existing arena-cached queries that currently return `&Option<T>`: `mir_coroutine_witnesses` and `diagnostic_hir_wf_check`. Only the query declarations need to be modified; existing providers and callers continue to work with the new query return type. (My real goal is to apply this to `coverage_ids_info`, which will return Option as of rust-lang#135873, but that PR hasn't landed yet.) [^deref]: Technically they could return other types that implement `Deref`, but it's hard to imagine this working well with anything other than `&T`.
These types are unlikely to ever contain type information in the foreseeable future, so excluding them from TypeFoldable/TypeVisitable avoids some unhelpful derive boilerplate.
This reflects the fact that we can't compute meaningful info for a function that wasn't instrumented and therefore doesn't have `function_coverage_info`.
Using `SmallVec` here was fine when it was a module-private detail, but if we want to pass these terms across query boundaries then it's not worth the extra hassle. Replacing a method call with direct field access is slightly simpler. Using the name `counter_terms` is more consistent with other code that tries to maintain a distinction between (physical) "counters" and "expressions".
This dedicated type seemed like a good idea at the time, but if we want to store this information in a query result then a plainer data type is more convenient.
By removing all methods from this struct and treating it as a collection of data fields, we make it easier for a future PR to store that data in a query result, without having to move all of its methods into `rustc_middle`.
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 24, 2025
coverage: Prepare for upcoming changes to counter creation This is a collection of smaller changes to coverage instrumentation code that have been extracted from a larger PR that I'm still working on, in order to hopefully make review easier. Each individual change should hopefully be mostly self-explanatory. One of the big goals of the upcoming PR will be to defer certain parts of counter-creation until codegen, via the query system, so that ends up being a recurring theme in these changes. Several of the changes are follow-ups to rust-lang#135481. There should be no observable change in compiler output.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2025
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#135295 (Check empty SIMD vector in inline asm) - rust-lang#135873 (coverage: Prepare for upcoming changes to counter creation) - rust-lang#135926 (Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}`) - rust-lang#135950 (Tidy Python improvements) - rust-lang#135956 (Make `Vec::pop_if` a bit more presentable) - rust-lang#135966 ([AIX] Allow different sized load and store in `tests/assembly/powerpc64-struct-abi.rs`) - rust-lang#135983 (Doc difference between extend and extend_from_slice) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2025
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#135873 (coverage: Prepare for upcoming changes to counter creation) - rust-lang#135926 (Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}`) - rust-lang#135950 (Tidy Python improvements) - rust-lang#135956 (Make `Vec::pop_if` a bit more presentable) - rust-lang#135966 ([AIX] Allow different sized load and store in `tests/assembly/powerpc64-struct-abi.rs`) - rust-lang#135983 (Doc difference between extend and extend_from_slice) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2025
Rollup merge of rust-lang#135873 - Zalathar:be-prepared, r=oli-obk coverage: Prepare for upcoming changes to counter creation This is a collection of smaller changes to coverage instrumentation code that have been extracted from a larger PR that I'm still working on, in order to hopefully make review easier. Each individual change should hopefully be mostly self-explanatory. One of the big goals of the upcoming PR will be to defer certain parts of counter-creation until codegen, via the query system, so that ends up being a recurring theme in these changes. Several of the changes are follow-ups to rust-lang#135481. There should be no observable change in compiler output.
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Feb 10, 2025
coverage: Defer part of counter-creation until codegen Follow-up to rust-lang#135481 and rust-lang#135873. One of the pleasant properties of the new counter-assignment algorithm is that we can stop partway through the process, store the intermediate state in MIR, and then resume the rest of the algorithm during codegen. This lets it take into account which parts of the control-flow graph were eliminated by MIR opts, resulting in fewer physical counters and simpler counter expressions. Those improvements end up completely obsoleting much larger chunks of code that were previously responsible for cleaning up the coverage metadata after MIR opts, while also doing a more thorough cleanup job. (That change also unlocks some further simplifications that I've kept out of this PR to limit its scope.)
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Feb 10, 2025
coverage: Defer part of counter-creation until codegen Follow-up to rust-lang#135481 and rust-lang#135873. One of the pleasant properties of the new counter-assignment algorithm is that we can stop partway through the process, store the intermediate state in MIR, and then resume the rest of the algorithm during codegen. This lets it take into account which parts of the control-flow graph were eliminated by MIR opts, resulting in fewer physical counters and simpler counter expressions. Those improvements end up completely obsoleting much larger chunks of code that were previously responsible for cleaning up the coverage metadata after MIR opts, while also doing a more thorough cleanup job. (That change also unlocks some further simplifications that I've kept out of this PR to limit its scope.)
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Feb 10, 2025
coverage: Defer part of counter-creation until codegen Follow-up to rust-lang#135481 and rust-lang#135873. One of the pleasant properties of the new counter-assignment algorithm is that we can stop partway through the process, store the intermediate state in MIR, and then resume the rest of the algorithm during codegen. This lets it take into account which parts of the control-flow graph were eliminated by MIR opts, resulting in fewer physical counters and simpler counter expressions. Those improvements end up completely obsoleting much larger chunks of code that were previously responsible for cleaning up the coverage metadata after MIR opts, while also doing a more thorough cleanup job. (That change also unlocks some further simplifications that I've kept out of this PR to limit its scope.)
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 10, 2025
Rollup merge of rust-lang#136053 - Zalathar:defer-counters, r=saethlin coverage: Defer part of counter-creation until codegen Follow-up to rust-lang#135481 and rust-lang#135873. One of the pleasant properties of the new counter-assignment algorithm is that we can stop partway through the process, store the intermediate state in MIR, and then resume the rest of the algorithm during codegen. This lets it take into account which parts of the control-flow graph were eliminated by MIR opts, resulting in fewer physical counters and simpler counter expressions. Those improvements end up completely obsoleting much larger chunks of code that were previously responsible for cleaning up the coverage metadata after MIR opts, while also doing a more thorough cleanup job. (That change also unlocks some further simplifications that I've kept out of this PR to limit its scope.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a collection of smaller changes to coverage instrumentation code that have been extracted from a larger PR that I'm still working on, in order to hopefully make review easier.
Each individual change should hopefully be mostly self-explanatory. One of the big goals of the upcoming PR will be to defer certain parts of counter-creation until codegen, via the query system, so that ends up being a recurring theme in these changes. Several of the changes are follow-ups to #135481.
There should be no observable change in compiler output.