[WIP] Working expression optimization, and some improvements to branch-level source coverage #78040
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
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @tmandry |
|
(Temporarily removed from "Draft" status to re-assign the reviewer, since I forgot to do that when originally posted.) |
richkadel
force-pushed
the
llvm-coverage-counters-2.0.3
branch
3 times, most recently
from
c30a19d
to
e302629
Compare
|
@tmandry @wesleywiser - I'm marking this ready for review even though there is still one function I will be refactoring some more ( As with the previous PR, there are a lot of changed files, but almost all of them are just re-generated test results. The primary goal of this PR was to convert counters to expressions as optimally as I could. It seems to be working. Existing tests pass. (I should add at least one more test with nested loops to prove the algorithm picks the best BasicCoverageBlock for the Expresion.) This turned out to be more complicated than I first anticipated, so I had to implement a lot of debugging features (which should also help future maintainers). |
|
FYI, I found a few bugs while trying to compile with real world crates. It took a day or so to fix them, and I added tests for these cases. As a result, I wasn't able to focus on the refactoring yet. I'll upload the fixes and then focus on the refactoring of the |
Starting with implementing the Graph traits for the BasicCoverageBlock graph.
Most of these TODO comments were reminders to remove commented out lines that were saved but replaced with an alternative implementation. Now that the new BCB graph is implemented and tests still pass, it should be safe to remove the older attempts.
I need to change how I determine if a target from a SwitchInt exits the loop or not, but this almost worked (and did successfully compile and create the right coverage). It just didn't select the right targets for the optimized expressions.
Still not fully optimized to remove unneeded expressions, or expressions
that complicate the MIR a bit without really improving coverage or
efficiency.
Getting some coverage results that don't seem right:
* In three tests, the last line of the function (last brace) is
counted twice.
* In simple_match, the { let z; match is counted zero times (and used
to be uncovered) but how would a developer ever get non-zero coverage
for this code?
* Avoid adding coverage to unreachable blocks. * Special case for Goto at the end of the body. Make it non-reportable.
They may still get counters but only if there are dependencies from other BCBs that have spans, I think. At least one test, "various_conditions.rs", seems to have simplified counters as a result of this change, for whatever reason.
I relized the "hack" with GapRegions was completely unnecessary. It is possible to inject counters (`llvm.instrprof.increment` intrinsic calls without corresponding code regions in the coverage map. An expression can still uses these counter values, solving the problem I thought I needed GapRegions for.
Just moved the file, before refactoring into multiple files in the new compiler/rustc_mir/src/transform/instrument_coverage/ directory.
Still working on make_bcb_counters() refacoring to split it into separate functions.
I should have re-tested for these, in the last PR. Here are those updates.
richkadel
force-pushed
the
llvm-coverage-counters-2.0.3
branch
2 times, most recently
from
ef27aac
to
3c176f4
Compare
Compiling with coverage, with the expression optimization, now works on the json5format crate and its dependencies.
richkadel
force-pushed
the
llvm-coverage-counters-2.0.3
branch
from
3c176f4
to
334bbbe
Compare
Also removed the SIMPLIFY_EXPRESSIONS option, which added complexity without tangible benefits.
It could be refactored even further, if that's preferred/requested, but its much better than it was ;-).
|
@wesleywiser @tmandry - I'm basically done proactively refactoring. If there is anything you'd like broken down further, let me know. Note, Tyler asked me to "reorganize the PR changes into commits which each do one "logical" thing", which I agreed to attempt. I'll start that tomorrow, so be on the lookout. I may create a new PR for it, just to make sure I don't accidentally lose some changes along the way. |
|
Replaced by #78267 |
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Nov 3, 2020
…0.3r1, r=tmandry Working expression optimization, and some improvements to branch-level source coverage This replaces PR rust-lang#78040 after reorganizing the original commits (by request) into a more logical sequence of major changes. Most of the work is in the MIR `transform/coverage/` directory (originally, `transform/instrument_coverage.rs`). Note this PR includes some significant additional debugging capabilities, to help myself and any future developer working on coverage improvements or issues. In particular, there's a new Graphviz (.dot file) output for the coverage graph (the `BasicCoverageBlock` control flow graph) that provides ways to get some very good insight into the relationships between the MIR, the coverage graph BCBs, coverage spans, and counters. (There are also some cool debugging options, available via environment variable, to alter how some data in the graph appears.) And the code for this Graphviz view is actually generic... it can be used by any implementation of the Rust `Graph` traits. Finally (for now), I also now output information from `llvm-cov` that shows the actual counters and spans it found in the coverage map, and their counts (from the `--debug` flag). I found this to be enormously helpful in debugging some coverage issues, so I kept it in the test results as well for additional context. `@tmandry` `@wesleywiser` r? `@tmandry` Here's an example of the new coverage graph: * Within each `BasicCoverageBlock` (BCB), you can see each `CoverageSpan` and its contributing statements (MIR `Statement`s and/or `Terminator`s) * Each `CoverageSpan` has a `Counter` or and `Expression`, and `Expression`s show their Add/Subtract operation with nested operations. (This can be changed to show the Counter and Expression IDs instead, or in addition to, the BCB.) * The terminators of all MIR `BasicBlock`s in the BCB, including one final `Terminator` * If an "edge counter" is required (because we need to count an edge between blocks, in some cases) the edge's Counter or Expression is shown next to its label. (Not shown in the example below.) (FYI, Edge Counters are converted into a new MIR `BasicBlock` with `Goto`) <img width="1116" alt="Screen Shot 2020-10-17 at 12 23 29 AM" src="https://user-images.githubusercontent.com/3827298/96331095-616cb480-100f-11eb-8212-60f2d433e2d8.png"> r? `@tmandry` FYI: `@wesleywiser`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 6, 2020
…3r1, r=tmandry Working expression optimization, and some improvements to branch-level source coverage This replaces PR rust-lang#78040 after reorganizing the original commits (by request) into a more logical sequence of major changes. Most of the work is in the MIR `transform/coverage/` directory (originally, `transform/instrument_coverage.rs`). Note this PR includes some significant additional debugging capabilities, to help myself and any future developer working on coverage improvements or issues. In particular, there's a new Graphviz (.dot file) output for the coverage graph (the `BasicCoverageBlock` control flow graph) that provides ways to get some very good insight into the relationships between the MIR, the coverage graph BCBs, coverage spans, and counters. (There are also some cool debugging options, available via environment variable, to alter how some data in the graph appears.) And the code for this Graphviz view is actually generic... it can be used by any implementation of the Rust `Graph` traits. Finally (for now), I also now output information from `llvm-cov` that shows the actual counters and spans it found in the coverage map, and their counts (from the `--debug` flag). I found this to be enormously helpful in debugging some coverage issues, so I kept it in the test results as well for additional context. `@tmandry` `@wesleywiser` r? `@tmandry` Here's an example of the new coverage graph: * Within each `BasicCoverageBlock` (BCB), you can see each `CoverageSpan` and its contributing statements (MIR `Statement`s and/or `Terminator`s) * Each `CoverageSpan` has a `Counter` or and `Expression`, and `Expression`s show their Add/Subtract operation with nested operations. (This can be changed to show the Counter and Expression IDs instead, or in addition to, the BCB.) * The terminators of all MIR `BasicBlock`s in the BCB, including one final `Terminator` * If an "edge counter" is required (because we need to count an edge between blocks, in some cases) the edge's Counter or Expression is shown next to its label. (Not shown in the example below.) (FYI, Edge Counters are converted into a new MIR `BasicBlock` with `Goto`) <img width="1116" alt="Screen Shot 2020-10-17 at 12 23 29 AM" src="https://user-images.githubusercontent.com/3827298/96331095-616cb480-100f-11eb-8212-60f2d433e2d8.png"> r? `@tmandry` FYI: `@wesleywiser`
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 draft work in progress.
It looks like it is working as I would expect. All existing tests pass. (There are a few small changes, improving the coverage from the last PR.)
Most of the work is in the MIR
transform/instrument_coverage.rs. I expanded this a lot, and it's too long right now, so before any serious code reviews, please wait for an update where I'll do some code restructuring to origanize things better. I'll most likely split this into a few separate files.I'll add more details soon, to describe the major changes.
Note this PR includes some significant additional debugging capabilities, to help myself and any future developer working on coverage improvements or issues.
In particular, there's a new Graphviz (.dot file) output for the coverage graph (the
BasicCoverageBlockcontrol flow graph) that provides ways to get some very good insight into the relationships between the MIR, the coverage graph BCBs, coverage spans, and counters. (There are also some cool debugging options, available via environment variable, to alter how some data in the graph appears.)And the code for this Graphviz view is actually generic... it can be used by any implementation of the Rust
Graphtraits.Finally (for now), I also now output information from
llvm-covthat shows the actual counters and spans it found in the coverage map, and their counts (from the--debugflag). I found this to be enormously helpful in debugging some coverage issues, so I kept it in the test results as well for additional context.@tmandry @wesleywiser
r? @tmandry
Here's an example of the new coverage graph:
BasicCoverageBlock(BCB), you can see eachCoverageSpanand its contributing statements (MIRStatements and/orTerminators)CoverageSpanhas aCounteror andExpression, andExpressions show their Add/Subtract operation with nested operations. (This can be changed to show the Counter and Expression IDs instead, or in addition to, the BCB.)BasicBlocks in the BCB, including one finalTerminatorBasicBlockwithGoto)