Move Coverage Regions / Expressions to a side table #115170
Closed
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
…LLVM We compile each test file to LLVM IR assembly, and then pass that IR to a dedicated program that can decode LLVM coverage maps and print them in a more human-readable format. We can then check that output against known-good snapshots. This test suite has some advantages over the existing `run-coverage` tests: - We can test coverage instrumentation without needing to run target binaries. - We can observe subtle improvements/regressions in the underlying coverage mappings that don't make a visible difference to coverage reports.
The output of these tests is too complicated to comfortably verify by hand, but we can still use them to observe changes to the underlying mappings produced by codegen/LLVM.
After coverage instrumentation and MIR transformations, we can sometimes end up with coverage expressions that always have a value of zero. Any expression operand that refers to an always-zero expression can be replaced with a literal `Operand::Zero`, making the emitted coverage mapping data smaller and simpler. This simplification step is mostly redundant with the simplifications performed inline in `expressions_with_regions`, except that it does a slightly more thorough job in some cases (because it checks for always-zero expressions *after* other simplifications). However, adding this simplification step will then let us greatly simplify that code, without affecting the quality of the emitted coverage maps.
The LLVM API that we use to encode coverage mappings already has its own code for removing unused coverage expressions and renumbering the rest. This lets us get rid of our own complex renumbering code, making it easier to refactor our coverage code in other ways.
This extends the current simplification code to not only replace operands by `Zero`, but also to remove trivial `Counter + Zero` expressions and replace those with just `Counter`. Currently this simplification is very simplistic, and does not handle more complex nested expressions such as `(A + B) - B` which could in theory be simplified as well.
Coverage statements in MIR are heavily tied to internal details of the coverage implementation that are likely to change, and are unlikely to be useful to third-party tools for the foreseeable future.
This removes quite a bit of indirection and duplicated code related to getting the `FunctionCoverage`.
Instead of encoding `CodeRegion`s and `CoverageExpression`s directly in a MIR `Coverage` statement, this moves this metadata to a side table. The `Coverage` statement is now only used to inject physical counters during codegen.
|
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-testsuite
rustbot
added
A-code-coverage
|
We've discussed this a bit in Zulip, and while conceptually this is a good change (I've been planning to do something similar for a while), I don't think it should happen right now. There are a few conflicting changes that I want to make to instrumentation and codegen first. Once those are taken care of, we can have another look at whether it's the right time to make the jump to coverage side-tables. |
|
☔ The latest upstream changes (presumably #115183) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing this in favor of #116046 which solves some of the shortcoming with my PR. |
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.
Instead of encoding
CodeRegions andCoverageExpressions directly in a MIRCoveragestatement, this metadata is moved to a side table.The
Coveragestatement is now only used to inject physical counters during codegen.This simplifies the code that injects, and extracts this metadata. The
CoverageVisitoris being removed, as thecoverageinfodoes not need it anymore. The query could probably be further simplified, maybe removing it completely in favor of just accessing the side table directly.Another thing that was removed was special casing for coverage statements in dead code elimination. Due to this, coverage expressions are not optimized based on unreachable regions.