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

Move cmp_in_dominator_order out of graph dominator computation #132022

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Move cmp_in_dominator_order out of graph dominator computation #132022

merged 1 commit into from
Oct 22, 2024

Conversation

Zalathar
Copy link
Contributor

Dominator-order information is only needed for coverage graphs, and is easy enough to collect by just traversing the graph again.

This avoids wasted work when computing graph dominators for any other purpose.

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

rustbot commented Oct 22, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
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 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 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 22, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Zalathar
Copy link
Contributor Author

One of my other motivations for doing this is that in the future I plan to also store reverse_post_order in the coverage graph for other purposes anyway, and that's much easier if it isn't hidden away in the implementation details of computing dominators.

@Zalathar
Copy link
Contributor Author

There are no coverage benchmarks, but I'm curious to see whether this is a measurable improvement for the rest of the compiler.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 22, 2024
@bors
Copy link
Contributor

bors commented Oct 22, 2024

⌛ Trying commit 292afd8 with merge f8a4fac...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2024
@bors
Auto merge of rust-lang#132022 - Zalathar:dominator-order, r=<try>
f8a4fac 
Move `cmp_in_dominator_order` out of graph dominator computation

Dominator-order information is only needed for coverage graphs, and is easy enough to collect by just traversing the graph again.

This avoids wasted work when computing graph dominators for any other purpose.
@bors
Copy link
Contributor

bors commented Oct 22, 2024

☀️ Try build successful - checks-actions
Build commit: f8a4fac (f8a4fac07f8796c010c100bdea1181f3850dd1e7)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

This comment was marked as resolved.

Sign in to view
@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 22, 2024
@Zalathar
Copy link
Contributor Author

As I half-expected, any improvement is too small to see among the noise. In that case, no reason to avoid rollup.

@bors rollup=maybe

@Zalathar
Move cmp_in_dominator_order out of graph dominator computation
7f4dd9b 
Dominator-order information is only needed for coverage graphs, and is easy
enough to collect by just traversing the graph again.

This avoids wasted work when computing graph dominators for any other purpose.
@tmiasko
Copy link
Contributor

tmiasko commented Oct 22, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 22, 2024

📌 Commit 7f4dd9b has been approved by tmiasko

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 Oct 22, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 22, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2024
@bors
Auto merge of rust-lang#132035 - matthiaskrgr:rollup-ty1e4q0, r=matth…
86d69c7 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125205 (Fixup Windows verbatim paths when used with the `include!` macro)
 - rust-lang#131049 (Validate args are correct for `UnevaluatedConst`, `ExistentialTraitRef`/`ExistentialProjection`)
 - rust-lang#131549 (Add a note for `?` on a `impl Future<Output = Result<..>>` in sync function)
 - rust-lang#131731 (add `TestFloatParse` to `tools.rs` for bootstrap)
 - rust-lang#131732 (Add doc(plugins), doc(passes), etc. to INVALID_DOC_ATTRIBUTES)
 - rust-lang#132006 (don't stage-off to previous compiler when CI rustc is available)
 - rust-lang#132022 (Move `cmp_in_dominator_order` out of graph dominator computation)
 - rust-lang#132033 (compiletest: Make `line_directive` return a `DirectiveLine`)

r? `@ghost`
`@rustbot` modify labels: rollup
@klensy
Copy link
Contributor

klensy commented Oct 22, 2024

Perf report contains:

Warning ⚠️: The following benchmark(s) failed to build:

    rustc

shouldn't this be tried again to check if it was spurious?

cjgillot
cjgillot reviewed Oct 22, 2024
View reviewed changes
compiler/rustc_mir_transform/src/coverage/graph.rs
let reverse_post_order = graph::iterate::reverse_post_order(&this, this.start_node());
// The coverage graph is created by traversal, so all nodes are reachable.
assert_eq!(reverse_post_order.len(), this.num_nodes());
for (rank, bcb) in (0u32..).zip(reverse_post_order) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit : reverse_post_order.enumerate() ?
R=me either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The node indices being ranked are 32-bit values, so using u32 for the rank (instead of usize) was intentional.

@bors bors merged commit f5aa456 into rust-lang:master Oct 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2024
@rust-timer
Unrolled build for rust-lang#132022
363767d 
Rollup merge of rust-lang#132022 - Zalathar:dominator-order, r=tmiasko

Move `cmp_in_dominator_order` out of graph dominator computation

Dominator-order information is only needed for coverage graphs, and is easy enough to collect by just traversing the graph again.

This avoids wasted work when computing graph dominators for any other purpose.
@Zalathar Zalathar deleted the dominator-order branch October 22, 2024 21:22
@Zalathar
Copy link
Contributor Author

Perf report contains:

Warning ⚠️: The following benchmark(s) failed to build:

    rustc

shouldn't this be tried again to check if it was spurious?

It was extremely unlikely for this PR to have caused a timeout, especially since the stage 2 dist build succeeded. So I didn't bother.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

@tmiasko tmiasko tmiasko approved these changes

Assignees

@cjgillot cjgillot

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.
7 participants
@Zalathar @rustbot @rust-timer @bors @tmiasko @klensy @cjgillot