Replace dominators algorithm with simple Lengauer-Tarjan

[Mark-Simulacrum]

This PR replaces our dominators implementation with that of the simple Lengauer-Tarjan algorithm, which is (to my knowledge and research) the currently accepted 'best' algorithm. The more complex variant has higher constant time overheads, and Semi-NCA (which is arguably a variant of Lengauer-Tarjan too) is not the preferred variant by the first paper cited in the documentation comments: simple Lengauer-Tarjan "is less sensitive to pathological instances, we think it should be preferred where performance guarantees are important" - which they are for us.

This work originally arose from noting that the keccak benchmark spent a considerable portion of its time (both instructions and cycles) in the dominator computations, which sparked an interest in potentially optimizing that code. The current algorithm largely proves slow on long "parallel" chains where the nearest common ancestor lookup (i.e., the intersect function) does not quickly identify a root; it is also inherently a pointer-chasing algorithm so is relatively slow on modern CPUs due to needing to hit memory - though usually in cache - in a tight loop, which still costs several cycles.

This was replaced with a bitset-based algorithm, previously studied in literature but implemented directly from dataflow equations in our case, which proved to be a significant speed up on the keccak benchmark: 20% instruction count wins, as can be seen in this performance report. This algorithm is also relatively simple in comparison to other algorithms and is easy to understand. However, these performance results showed a regression on a number of other benchmarks, and I was unable to get the bitsets to perform well enough that those regressions could be fully mitigated. The implementation "attempt" is seen here in the first commit, and is intended to be kept primarily so that future optimizers do not repeat that path (or can easily refer to the attempt).

The final version of this PR chooses the simple Lengauer-Tarjan algorithm, and implements it along with a number of optimizations found in literature. The current implementation is a slight improvement for many benchmarks, with keccak still being an outlier at ~20%. The implementation in this PR first implements the most basic variant of the algorithm directly from the pseudocode on page 16, physical, or 28 in the PDF of the first paper ("Linear-Time Algorithms for Dominators and Related Problems"). This is then followed by a number of commits which update the implementation to apply various performance improvements, as suggested by the paper. Finally, the last commit annotates the implementation with a number of comments, mostly drawn from the paper, which intend to help readers understand what is going on - these are incomplete without the paper, but writing them certainly helped my understanding. They may be helpful if future optimization attempts are attempted, so I chose to add them in.

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit cb3a6691b559741596baf858174ce96c0551532e with merge 542da47ff78aa462384062229dad0675792f2638...

[bors]

☀️ Try build successful - checks-actions
Build commit: 542da47ff78aa462384062229dad0675792f2638 (542da47ff78aa462384062229dad0675792f2638)

[rust-timer]

Queued 542da47ff78aa462384062229dad0675792f2638 with parent 377d1a9, future comparison URL.

[rust-timer]

Finished benchmarking try commit (542da47ff78aa462384062229dad0675792f2638): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[Mark-Simulacrum]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit d4942af5bebba41a09ec1aafe90d90cb16d00734 with merge 791dd884482caba07863001c635d07caeb228262...

[bors]

☀️ Try build successful - checks-actions
Build commit: 791dd884482caba07863001c635d07caeb228262 (791dd884482caba07863001c635d07caeb228262)

[rust-timer]

Queued 791dd884482caba07863001c635d07caeb228262 with parent e5f83d2, future comparison URL.

[rust-timer]

Finished benchmarking try commit (791dd884482caba07863001c635d07caeb228262): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[Mark-Simulacrum]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 8fc6295edcc20997ad6565760ce2488971c98c91 with merge b059578157e5fd1e0cca4c4853ba9a642288ede5...

[bors]

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

[bors]

⌛ Trying commit 7e435ffdd7e765bd0e3619cd75c43ac6ef6e998b with merge 8d61ef5e235e84fb7a2844719ebab57784ff3586...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8d61ef5e235e84fb7a2844719ebab57784ff3586 (8d61ef5e235e84fb7a2844719ebab57784ff3586)

[rust-timer]

Queued 8d61ef5e235e84fb7a2844719ebab57784ff3586 with parent 0fb1c37, future comparison URL.

[rust-timer]

Finished benchmarking commit (8d61ef5e235e84fb7a2844719ebab57784ff3586): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

• Very large improvement in instruction counts (up to -23.6% on full builds of keccak)
• Small regression in instruction counts (up to 0.4% on incr-patched: println builds of style-servo)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Mark-Simulacrum]

@bors r=pnkfelix

Perf results are still good, with small regressions compared to large wins. I've squashed in the suggestions so I think this should be good to go ahead.

[bors]

📌 Commit 15483cc has been approved by pnkfelix

[bors]

⌛ Testing commit 15483cc with merge c67497a...

[bors]

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing c67497a to master...

[rust-timer]

Finished benchmarking commit (c67497a): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

• Very large improvement in instruction counts (up to -23.6% on full builds of keccak)
• Small regression in instruction counts (up to 0.4% on incr-unchanged builds of helloworld)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[pnkfelix]

I think most of the regressing cases are in the noise here.

(Of course, keccak is not typical code either. I'm not expecting everyone to get 23% faster compiles with this change. But the motivation and reasoning behind it remain sound.

@rustbot label: +perf-regression-triaged

[nnethercote]

Interesting that inflate didn't improve here. My experience is that inflate and keccak tend to improve/regress together because they do (warning: technical terms ahead) tons of ObligationForest stuff.