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

coverage: Simplify parts of coverage graph creation #132389

Merged
merged 3 commits into from
Nov 8, 2024
Merged

coverage: Simplify parts of coverage graph creation #132389

merged 3 commits into from
Nov 8, 2024

Conversation

Zalathar
Copy link
Contributor

This is a combination of three semi-related simplifications to how coverage graphs are created, grouped into one PR to avoid conflicts.

There are no observable changes to the output of any of the coverage tests.

@Zalathar Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Oct 31, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

r? @saethlin

rustbot has assigned @saethlin.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 31, 2024
@Zalathar Zalathar mentioned this pull request Oct 31, 2024
Closed
@Zalathar
Copy link
Contributor Author

Notes on the CoverageSuccessors change, from #132379:

There are two minor differences implied by this change:

  • A SwitchInt terminator with exactly one target is now eligible to be chained.
    • The previous behaviour didn't have any strong rationale, and this gets rid of a special case.
  • A terminator with no coverage-relevant successors is now always marked as “not out-summable”.
    • This is more correct, because (for example) a Return terminator has no successors (so their sum of execution counts is 0), but the return itself can have a non-zero execution count.
    • The existing code that checks this flag doesn't care either way, but this is likely to matter in the future.

@Zalathar
coverage: Simplify logic for chaining multiple blocks into one BCB
4a70f4b 
We only need to take action when the next block cannot be added to the current
chain, but the logic is much simpler if we express it in terms of when the
block _can_ be added.
@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 1, 2024

While we're here, might as well shorten std::mem::take to mem::take (diff).

@jieyouxu
Copy link
Member

jieyouxu commented Nov 7, 2024

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned saethlin Nov 7, 2024
jieyouxu
jieyouxu approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks looks like a nice cleanup, thanks!

@jieyouxu
Copy link
Member

jieyouxu commented Nov 8, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 8, 2024

📌 Commit 4a70f4b has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2024
@Zalathar Zalathar mentioned this pull request Nov 8, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
@bors
Auto merge of rust-lang#132762 - Zalathar:rollup-qfgz165, r=Zalathar
6295686 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#132161 ([StableMIR] A few fixes to pretty printing)
 - rust-lang#132389 (coverage: Simplify parts of coverage graph creation)
 - rust-lang#132452 (coverage: Extract safe FFI wrapper functions to `llvm_cov`)
 - rust-lang#132590 (Simplify FFI calls for `-Ztime-llvm-passes` and `-Zprint-codegen-stats`)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1b55244 into rust-lang:master Nov 8, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 8, 2024
@Zalathar Zalathar deleted the graph-tweaks branch November 8, 2024 10:28
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
@rust-timer
Unrolled build for rust-lang#132389
11da1b2 
Rollup merge of rust-lang#132389 - Zalathar:graph-tweaks, r=jieyouxu

coverage: Simplify parts of coverage graph creation

This is a combination of three semi-related simplifications to how coverage graphs are created, grouped into one PR to avoid conflicts.

There are no observable changes to the output of any of the coverage tests.
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
@Zalathar @mati865
Rollup merge of rust-lang#132389 - Zalathar:graph-tweaks, r=jieyouxu
0c14074 
coverage: Simplify parts of coverage graph creation

This is a combination of three semi-related simplifications to how coverage graphs are created, grouped into one PR to avoid conflicts.

There are no observable changes to the output of any of the coverage tests.
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
@bors @mati865
Auto merge of rust-lang#132762 - Zalathar:rollup-qfgz165, r=Zalathar
b56f1c1 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#132161 ([StableMIR] A few fixes to pretty printing)
 - rust-lang#132389 (coverage: Simplify parts of coverage graph creation)
 - rust-lang#132452 (coverage: Extract safe FFI wrapper functions to `llvm_cov`)
 - rust-lang#132590 (Simplify FFI calls for `-Ztime-llvm-passes` and `-Zprint-codegen-stats`)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Zalathar @rustbot @jieyouxu @bors @saethlin