Move cmp_in_dominator_order out of graph dominator computation by Zalathar · Pull Request #132022 · rust-lang/rust (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks6 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
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.
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Some changes occurred to MIR optimizations
cc @rust-lang/wg-mir-opt
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.
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
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
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.
☀️ Try build successful - checks-actions
Build commit: f8a4fac (f8a4fac07f8796c010c100bdea1181f3850dd1e7)
This comment has been minimized.
This comment was marked as resolved.
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
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.
📌 Commit 7f4dd9b has been approved by tmiasko
It is now in the queue for this repository.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…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 aimpl Future<Output = Result<..>>in sync function) - rust-lang#131731 (add
TestFloatParsetotools.rsfor 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_orderout of graph dominator computation) - rust-lang#132033 (compiletest: Make
line_directivereturn aDirectiveLine)
r? @ghost
@rustbot modify labels: rollup
Perf report contains:
Warning ⚠️: The following benchmark(s) failed to build:
rustc
shouldn't this be tried again to check if it was spurious?
| 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) { |
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
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.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
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.
Perf report contains:
Warning ⚠️: The following benchmark(s) failed to build: rustcshouldn'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.
Labels
Area: Source-based code coverage (-Cinstrument-coverage)
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.