coverage: Don't bother renumbering expressions on the Rust side #114399
Conversation
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@rustbot label +A-code-coverage |
rustbot
added
the
A-code-coverage
Zalathar
force-pushed
the
no-renumber
branch
2 times, most recently
from
71d2089
to
f7f8123
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Zalathar
force-pushed
the
no-renumber
branch
5 times, most recently
from
b26bbf3
to
24ad7c5
Compare
|
@Zalathar I'll wait to review this for a bit. Let me know if the other PR takes more time than you want, or you'd rather I just review this. |
|
@jackh726 Yeah, currently the other change is proceeding well, so I think it makes sense to wait on reviewing this one. I'll speak up when that changes. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
rustbot
added
A-testsuite
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
I've rebased this to sit on top of a copy of #114843, which adds the new tests that demonstrate how this affects coverage maps. Once that gets merged and I rebase this onto the new master, the patches with 🍒 should all disappear. Using those tests, I've found that there are changes to the maps, but they can be eliminated by adding an explicit simplify-expressions step during coverage codegen that replicates the simplifications performed in-passing by the renumbering code. (In fact, it does a slightly better job in some cases.) |
Zalathar
force-pushed
the
no-renumber
branch
12 times, most recently
from
6c86faf
to
72721d2
Compare
Zalathar
force-pushed
the
no-renumber
branch
2 times, most recently
from
2c6a213
to
4a8f6cf
Compare
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 change our coverage code in other ways.
Zalathar
force-pushed
the
no-renumber
branch
from
4a8f6cf
to
041a232
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (24b45c3): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 630.615s -> 633.468s (0.45%) |
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 change our coverage code in other ways.
Now that we have tests for coverage mappings (#114843), I've been able to verify that this PR doesn't make the coverage mappings worse, thanks to an explicit simplification step.