Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generalize Coverage Expression simplification #115134

Closed
wants to merge 7 commits into from
Closed

Generalize Coverage Expression simplification #115134

wants to merge 7 commits into from

Conversation

Swatinem
Copy link
Contributor

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.

This is based on top of #114399 CC @Zalathar

Zalathar and others added 7 commits August 23, 2023 22:51
@Zalathar
🍒 Add tool src/tools/coverage-dump for use by some new coverage tests
cc832c5
@Zalathar
🍒 Add test suite coverage-map to test coverage mappings emitted by …
d348d9f 
…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.
@Zalathar
🍒 Copy a selection of run-coverage test files into coverage-map
05b398e 
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.
@Zalathar
coverage: Make the zero counter a constant
f268c0a
@Zalathar
coverage: Explicitly simplify coverage expressions in codegen
f39b246 
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.
@Zalathar
coverage: Don't bother renumbering expressions on the Rust side
5e63f64 
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.
@Swatinem
Generalize Coverage Expression simplification
bae94ee 
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.
@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@Swatinem
Copy link
Contributor Author

@rustbot label +A-code-coverage

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Aug 23, 2023
@Zalathar
Copy link
Contributor

@rustbot blocked

(This is written on top of #114399, so it can't land until that PR lands.)

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2023
@Zalathar
Copy link
Contributor

I think we shouldn't add more simplifications right now, for a few reasons:

  • Simpler mappings are nice to have, but they aren't essential; coverage works fine even if the mappings are unnecessarily complex. So if simplification interferes with other priorities, then those should take precedence.
  • My goal behind coverage: Don't bother renumbering expressions on the Rust side #114399 is to work towards being able to make big changes to how coverage info is stored in MIR. To make that happen, all of the related code needs to be as simple as possible, so that it's feasible for me to make the necessary changes in one go. Adding more simplification code right now would get in the way of that.
  • A lot of the unnecessary complexity in mappings comes from the instrumentor deliberately adding (x + 0) expressions instead of using the same counter/expression twice. That's currently necessary (due to other design decisions in the original coverage implementation), and I'd like to fix that at the source rather than papering over it in codegen.
  • Aside from the above, many of the opportunities for simplification come from the fact that MIR optimizations can remove CoverageKind::Expression nodes. When I eventually move most of the coverage data out of MIR statements and into a per-function structure, the simplification steps will need to work differently, so putting effort into them now will end up being a bit of a waste.

I did add some simplification code in 114399, but that was only so that I could remove expression renumbering without making the mappings worse (because the renumbering code also performed a couple of simplifications of its own).

@Swatinem Swatinem mentioned this pull request Aug 24, 2023
Closed
@bors
Copy link
Contributor

bors commented Aug 29, 2023

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

@Swatinem
Copy link
Contributor Author

Other work on the coverage mappings have made this unnecessary

@Swatinem Swatinem closed this Sep 22, 2023
@Swatinem Swatinem deleted the further-simplify-coverage-expr branch May 25, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@b-naber b-naber

Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-testsuite Area: The testsuite used to check the correctness of rustc S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Swatinem @rustbot @Zalathar @bors @b-naber